Message ID | c3f3e573-8f5c-8dca-7db7-b804878a8716@redhat.com |
---|---|
State | New |
Headers | show |
Series | [committed,PR99454] LRA: Process 0..9 constraints in process_address_1 | expand |
On Tue, Mar 09, 2021 at 09:12:36AM -0500, Vladimir Makarov via Gcc-patches wrote: > diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c > index 76e3ff7efe6..feff766c590 100644 > --- a/gcc/lra-constraints.c > +++ b/gcc/lra-constraints.c > @@ -3452,6 +3452,10 @@ process_address_1 (int nop, bool check_only_p, > > constraint > = skip_contraint_modifiers (curr_static_id->operand[nop].constraint); > + if ('0' <= constraint[0] && constraint[0] <= '9') Yoda conditions? Anyway, more importantly, the docs say: If multiple digits are encountered consecutively, they are interpreted as a single decimal integer. There is scant chance for ambiguity, since to-date it has never been desirable that '10' be interpreted as matching either operand 1 _or_ operand 0. Should this be desired, one can use multiple alternatives instead. so, doesn't this need to use { char **end; unsigned long dup = strtoul (constraint, &end, 10); constraint = skip_constraint_modifiers (curr_static_id_operand[dup].constraint); } instead? Otherwise, if you have (even large) testcase that misbehaves with 0-9 dups, it should be possible to create one with 10-30? > + constraint > + = skip_contraint_modifiers (curr_static_id->operand > + [constraint[0] - '0'].constraint); > cn = lookup_constraint (constraint); > if (insn_extra_address_constraint (cn) > /* When we find an asm operand with an address constraint that Jakub
On Tue, Mar 09, 2021 at 03:26:12PM +0100, Jakub Jelinek via Gcc-patches wrote: > On Tue, Mar 09, 2021 at 09:12:36AM -0500, Vladimir Makarov via Gcc-patches wrote: > > diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c > > index 76e3ff7efe6..feff766c590 100644 > > --- a/gcc/lra-constraints.c > > +++ b/gcc/lra-constraints.c > > @@ -3452,6 +3452,10 @@ process_address_1 (int nop, bool check_only_p, > > > > constraint > > = skip_contraint_modifiers (curr_static_id->operand[nop].constraint); > > + if ('0' <= constraint[0] && constraint[0] <= '9') > > Yoda conditions? We have IN_RANGE (constraint[0], '0', '9') for this. > Anyway, more importantly, the docs say: > If multiple > digits are encountered consecutively, they are interpreted as a > single decimal integer. There is scant chance for ambiguity, since > to-date it has never been desirable that '10' be interpreted as > matching either operand 1 _or_ operand 0. Should this be desired, > one can use multiple alternatives instead. > so, doesn't this need to use > { > char **end; > unsigned long dup = strtoul (constraint, &end, 10); > constraint > = skip_constraint_modifiers (curr_static_id_operand[dup].constraint); > } > instead? > Otherwise, if you have (even large) testcase that misbehaves with 0-9 > dups, it should be possible to create one with 10-30? > > > + constraint > > + = skip_contraint_modifiers (curr_static_id->operand > > + [constraint[0] - '0'].constraint); > > cn = lookup_constraint (constraint); > > if (insn_extra_address_constraint (cn) > > /* When we find an asm operand with an address constraint that > > > Jakub > Marek
On 2021-03-09 9:26 a.m., Jakub Jelinek wrote: > On Tue, Mar 09, 2021 at 09:12:36AM -0500, Vladimir Makarov via Gcc-patches wrote: >> diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c >> index 76e3ff7efe6..feff766c590 100644 >> --- a/gcc/lra-constraints.c >> +++ b/gcc/lra-constraints.c >> @@ -3452,6 +3452,10 @@ process_address_1 (int nop, bool check_only_p, >> >> constraint >> = skip_contraint_modifiers (curr_static_id->operand[nop].constraint); >> + if ('0' <= constraint[0] && constraint[0] <= '9') > Yoda conditions? > > Anyway, more importantly, the docs say: > If multiple > digits are encountered consecutively, they are interpreted as a > single decimal integer. There is scant chance for ambiguity, since > to-date it has never been desirable that '10' be interpreted as > matching either operand 1 _or_ operand 0. Should this be desired, > one can use multiple alternatives instead. > so, doesn't this need to use > { > char **end; > unsigned long dup = strtoul (constraint, &end, 10); > constraint > = skip_constraint_modifiers (curr_static_id_operand[dup].constraint); > } > instead? Yes, you are right. Thank you for pointing this out. I'll fix it soon. There are places where these constraints are treated the same which should be fixed too. They are not critical as they are used in cost calculation. > Otherwise, if you have (even large) testcase that misbehaves with 0-9 > dups, it should be possible to create one with 10-30? >
On 3/9/21 3:12 PM, Vladimir Makarov via Gcc-patches wrote:
> The patch was successfully bootstrapped and tested on x86-64, ppc64le, and arm64. Unfortunately, I did not manage to reduce the test (whose size is 5MB).
I've just reduced test-case from it:
$ cat a.c
struct skb_shared_info {
short gso_size;
};
enum { NETDEV_TX_OK };
struct iphdr {
short tot_len;
int daddr;
};
int tg3_tso_bug();
int netdev_priv();
int skb_cow_head();
int tcp_hdrlen__builtin_expect();
struct iphdr *ip_hdr();
int _tg3_flag();
int tg3_tso_bug_gso_check();
int
tg3_start_xmit() {
int *tp = netdev_priv();
int mss, tnapi;
struct iphdr *iph;
tnapi = mss = ((struct skb_shared_info *)0)->gso_size;
if (mss) {
int hdr_len;
if (skb_cow_head())
iph = ip_hdr();
hdr_len = tcp_hdrlen__builtin_expect() && _tg3_flag();
if (tg3_tso_bug_gso_check())
return tg3_tso_bug(tp, tnapi);
iph->tot_len = mss + hdr_len;
if (_tg3_flag(tp) || tp)
;
else
asm("" : : "g"(iph->daddr));
}
return 0;
}
Please add it into test-suite.
Thanks,
Martin
On 2021-03-09 9:53 a.m., Martin Liška wrote: > On 3/9/21 3:12 PM, Vladimir Makarov via Gcc-patches wrote: >> The patch was successfully bootstrapped and tested on x86-64, >> ppc64le, and arm64. Unfortunately, I did not manage to reduce the >> test (whose size is 5MB). > > I've just reduced test-case from it: > > $ cat a.c > struct skb_shared_info { > short gso_size; > }; > > enum { NETDEV_TX_OK }; > > struct iphdr { > short tot_len; > int daddr; > }; > > int tg3_tso_bug(); > int netdev_priv(); > int skb_cow_head(); > int tcp_hdrlen__builtin_expect(); > struct iphdr *ip_hdr(); > int _tg3_flag(); > int tg3_tso_bug_gso_check(); > > int > tg3_start_xmit() { > int *tp = netdev_priv(); > int mss, tnapi; > struct iphdr *iph; > tnapi = mss = ((struct skb_shared_info *)0)->gso_size; > if (mss) { > int hdr_len; > if (skb_cow_head()) > iph = ip_hdr(); > hdr_len = tcp_hdrlen__builtin_expect() && _tg3_flag(); > if (tg3_tso_bug_gso_check()) > return tg3_tso_bug(tp, tnapi); > iph->tot_len = mss + hdr_len; > if (_tg3_flag(tp) || tp) > ; > else > asm("" : : "g"(iph->daddr)); > } > return 0; > } > > Please add it into test-suite. > Thank you. Will do.
Here is the patch taking all the proposals into account. Successfully bootstrapped on x86-64.
commit 9725df0233b6fb6e761875968b3b8e9fd9f522ac (HEAD -> master) Author: Vladimir N. Makarov <vmakarov@redhat.com> Date: Tue Mar 9 09:05:05 2021 -0500 [PR99454] LRA: Process 0..9 constraints in process_address_1 We need to process 0..9 constraints to fetch the right op constraint in the function. Also 0..9 constraints gives unknown class constraint class which can result in skipping address normalization for memory in asm. gcc/ChangeLog: PR target/99454 * lra-constraints.c (process_address_1): Process 0..9 constraints in process_address_1. diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c index 76e3ff7efe6..feff766c590 100644 --- a/gcc/lra-constraints.c +++ b/gcc/lra-constraints.c @@ -3452,6 +3452,10 @@ process_address_1 (int nop, bool check_only_p, constraint = skip_contraint_modifiers (curr_static_id->operand[nop].constraint); + if ('0' <= constraint[0] && constraint[0] <= '9') + constraint + = skip_contraint_modifiers (curr_static_id->operand + [constraint[0] - '0'].constraint); cn = lookup_constraint (constraint); if (insn_extra_address_constraint (cn) /* When we find an asm operand with an address constraint that