| Message ID | HE1PR08MB05075A4CBF54BC86B9007F59E7BE0@HE1PR08MB0507.eurprd08.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Mar 4, 2016 at 12:07 PM, Bin Cheng <Bin.Cheng@arm.com> wrote: > Hi, > A address canonicalization interface was introduced by my original patch to PR69052. The interface sorts sub-parts in an address expression based on precedences defined by function commutative_operand_precedence. It also assumes that all CONST_INT sub-parts are at the end of vector after sorting. But this is not always true because commutative_operand_precedence sets the same precedence to both CONST_INT and CONST_WIDE_INT. The patch could be broken in cases which have CONST_WIDE_INT sorted after CONST_INT. Even though I don't think we can really run into such complicated case, I worked out a patch forcing CONST_INT to lower precedence than CONST_WIDE_INT, so that for sure it will be sorted after all other kinds sub-parts. > > This is an obvious change. Bootstrap&test on x86_64, bootstrap&test on AArch64. Is it OK for this stage? I believe the obvious change would be to change commutative_operand_precedence to pre-CONST_WIDE_INT behavior, namely giving CONST_WIDE_INT the same precedence as CONST_DOUBLE. Richard. > Thanks, > bin > > 2016-03-04 Bin Cheng <bin.cheng@arm.com> > > PR rtl-optimization/69052 > * loop-invariant.c (compare_address_parts): Force CONST_INT to lower > precedence than CONST_WIDE_INT.
On Fri, Mar 4, 2016 at 11:57 AM, Richard Biener <richard.guenther@gmail.com> wrote: > On Fri, Mar 4, 2016 at 12:07 PM, Bin Cheng <Bin.Cheng@arm.com> wrote: >> Hi, >> A address canonicalization interface was introduced by my original patch to PR69052. The interface sorts sub-parts in an address expression based on precedences defined by function commutative_operand_precedence. It also assumes that all CONST_INT sub-parts are at the end of vector after sorting. But this is not always true because commutative_operand_precedence sets the same precedence to both CONST_INT and CONST_WIDE_INT. The patch could be broken in cases which have CONST_WIDE_INT sorted after CONST_INT. Even though I don't think we can really run into such complicated case, I worked out a patch forcing CONST_INT to lower precedence than CONST_WIDE_INT, so that for sure it will be sorted after all other kinds sub-parts. >> >> This is an obvious change. Bootstrap&test on x86_64, bootstrap&test on AArch64. Is it OK for this stage? > > I believe the obvious change would be to change > commutative_operand_precedence to pre-CONST_WIDE_INT behavior, namely > giving CONST_WIDE_INT the same precedence as CONST_DOUBLE. Yes, but I am not sure what else this change will affect, so I made the smallest change in the original code. I am testing this now. It would be great if anyone describes it a bit. Thanks, bin > > Richard. > > > >> Thanks, >> bin >> >> 2016-03-04 Bin Cheng <bin.cheng@arm.com> >> >> PR rtl-optimization/69052 >> * loop-invariant.c (compare_address_parts): Force CONST_INT to lower >> precedence than CONST_WIDE_INT.
On March 4, 2016 5:35:13 PM GMT+01:00, "Bin.Cheng" <amker.cheng@gmail.com> wrote: >On Fri, Mar 4, 2016 at 11:57 AM, Richard Biener ><richard.guenther@gmail.com> wrote: >> On Fri, Mar 4, 2016 at 12:07 PM, Bin Cheng <Bin.Cheng@arm.com> wrote: >>> Hi, >>> A address canonicalization interface was introduced by my original >patch to PR69052. The interface sorts sub-parts in an address >expression based on precedences defined by function >commutative_operand_precedence. It also assumes that all CONST_INT >sub-parts are at the end of vector after sorting. But this is not >always true because commutative_operand_precedence sets the same >precedence to both CONST_INT and CONST_WIDE_INT. The patch could be >broken in cases which have CONST_WIDE_INT sorted after CONST_INT. Even >though I don't think we can really run into such complicated case, I >worked out a patch forcing CONST_INT to lower precedence than >CONST_WIDE_INT, so that for sure it will be sorted after all other >kinds sub-parts. >>> >>> This is an obvious change. Bootstrap&test on x86_64, bootstrap&test >on AArch64. Is it OK for this stage? >> >> I believe the obvious change would be to change >> commutative_operand_precedence to pre-CONST_WIDE_INT behavior, namely >> giving CONST_WIDE_INT the same precedence as CONST_DOUBLE. >Yes, but I am not sure what else this change will affect, so I made >the smallest change in the original code. I don't like going with this kind of weirdo changes out of caution. If you think it's too dangerous the push it back to gcc 7 and consider backporting for 6.2 I am testing this now. It >would be great if anyone describes it a bit. Thanks, Richard. > >Thanks, >bin >> >> Richard. >> >> >> >>> Thanks, >>> bin >>> >>> 2016-03-04 Bin Cheng <bin.cheng@arm.com> >>> >>> PR rtl-optimization/69052 >>> * loop-invariant.c (compare_address_parts): Force CONST_INT >to lower >>> precedence than CONST_WIDE_INT.
diff --git a/gcc/loop-invariant.c b/gcc/loop-invariant.c index dcbe932..27a1815 100644 --- a/gcc/loop-invariant.c +++ b/gcc/loop-invariant.c @@ -820,6 +820,12 @@ compare_address_parts (const void *x, const void *y) int px = commutative_operand_precedence (*rx); int py = commutative_operand_precedence (*ry); + /* Put CONST_INT behind CONST_WIDE_INT. */ + if (CONST_INT_P (*rx) && CONST_WIDE_INT_P (*ry)) + return 1; + else if (CONST_WIDE_INT_P (*rx) && CONST_INT_P (*ry)) + return -1; + return (py - px); }