| Message ID | 20210513104819.1081411-1-rearnsha@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [committed] arm: correctly handle inequality comparisons against max constants [PR100563] | expand |
On Thu, 13 May 2021, Richard Earnshaw via Gcc-patches wrote: > > Normally we expect the gimple optimizers to fold away comparisons that > are always true, but at some lower optimization levels this is not > always the case, so the back-end has to be able to generate correct > code in these cases. > > In this example, we have a comparison of the form > > (unsigned long long) op <= ~0ULL > > which, of course is always true. > > Normally, in the arm back-end we handle these expansions where the > immediate cannot be handled directly by adding 1 to the constant and > then adjusting the comparison operator: > > (unsigned long long) op < CONST + 1 > > but we cannot do that when the constant is already the largest value. Sounds like a target-independent bug in the making, lurking and waiting for a target to do the above adjustment but missing the bounds-check. > gcc/testsuite/gcc.dg/pr100563.c | 9 +++++++++ > 2 files changed, 34 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/pr100563.c I'll therefore humbly suggest the test-case adjusted to be a run-time check (and if not done by others, projecting to do that myself...some time late next summer9. brgds, H-P
On 17/05/2021 21:52, Hans-Peter Nilsson wrote: > On Thu, 13 May 2021, Richard Earnshaw via Gcc-patches wrote: >> >> Normally we expect the gimple optimizers to fold away comparisons that >> are always true, but at some lower optimization levels this is not >> always the case, so the back-end has to be able to generate correct >> code in these cases. >> >> In this example, we have a comparison of the form >> >> (unsigned long long) op <= ~0ULL >> >> which, of course is always true. >> >> Normally, in the arm back-end we handle these expansions where the >> immediate cannot be handled directly by adding 1 to the constant and >> then adjusting the comparison operator: >> >> (unsigned long long) op < CONST + 1 >> >> but we cannot do that when the constant is already the largest value. > > Sounds like a target-independent bug in the making, lurking and > waiting for a target to do the above adjustment but missing the > bounds-check. > The normal canonicalize_comparison operation tries to simplify expressions by reducing the value towards zero and making the appropriate adjustment to the comparison. Arm is a bit different due to the way constants are encoded and also because of the limitations on how we expand DImode in the backend. So I don't think this is quite as general as you suggest. >> gcc/testsuite/gcc.dg/pr100563.c | 9 +++++++++ >> 2 files changed, 34 insertions(+), 4 deletions(-) >> create mode 100644 gcc/testsuite/gcc.dg/pr100563.c > > I'll therefore humbly suggest the test-case adjusted to be a > run-time check (and if not done by others, projecting to do that > myself...some time late next summer9. That being said, I won't object if you want to /add/ an additional test to try to catch this; but this test had to be quite carefully crafted to hit the specific bug in the arm expanders, so I'd prefer that it wasn't modified directly. R. > > brgds, H-P >
On Tue, 18 May 2021, Richard Earnshaw wrote: > On 17/05/2021 21:52, Hans-Peter Nilsson wrote: > > On Thu, 13 May 2021, Richard Earnshaw via Gcc-patches wrote: > > > > > > Normally we expect the gimple optimizers to fold away comparisons that > > > are always true, but at some lower optimization levels this is not > > > always the case, so the back-end has to be able to generate correct > > > code in these cases. > > > > > > In this example, we have a comparison of the form > > > > > > (unsigned long long) op <= ~0ULL > > > > > > which, of course is always true. > > > > > > Normally, in the arm back-end we handle these expansions where the > > > immediate cannot be handled directly by adding 1 to the constant and > > > then adjusting the comparison operator: > > > > > > (unsigned long long) op < CONST + 1 > > > > > > but we cannot do that when the constant is already the largest value. > > > > Sounds like a target-independent bug in the making, lurking and > > waiting for a target to do the above adjustment but missing the > > bounds-check. > > The normal canonicalize_comparison operation tries to simplify expressions by > reducing the value towards zero and making the appropriate adjustment to the > comparison. Arm is a bit different due to the way constants are encoded and > also because of the limitations on how we expand DImode in the backend. So I > don't think this is quite as general as you suggest. It just seemed I had done something similar somewhere in another port, though I can't find it. I could also have only read it... > > > gcc/testsuite/gcc.dg/pr100563.c | 9 +++++++++ > > > 2 files changed, 34 insertions(+), 4 deletions(-) > > > create mode 100644 gcc/testsuite/gcc.dg/pr100563.c > > > > I'll therefore humbly suggest the test-case adjusted to be a > > run-time check (and if not done by others, projecting to do that > > myself...some time late next summer9. > > That being said, I won't object if you want to /add/ an additional test to try > to catch this; but this test had to be quite carefully crafted to hit the > specific bug in the arm expanders, so I'd prefer that it wasn't modified > directly. Right. I expressed myself poorly as I share your concerns on this point: after the public commit of a test that had no error, it must never be modified. To wit, such an "adjustment" must be made in the form of an additional, separate test. brgds, H-P
On Tue, May 18, 2021 at 1:26 PM Richard Earnshaw via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > > On 17/05/2021 21:52, Hans-Peter Nilsson wrote: > > On Thu, 13 May 2021, Richard Earnshaw via Gcc-patches wrote: > >> > >> Normally we expect the gimple optimizers to fold away comparisons that > >> are always true, but at some lower optimization levels this is not > >> always the case, so the back-end has to be able to generate correct > >> code in these cases. > >> > >> In this example, we have a comparison of the form > >> > >> (unsigned long long) op <= ~0ULL > >> > >> which, of course is always true. > >> > >> Normally, in the arm back-end we handle these expansions where the > >> immediate cannot be handled directly by adding 1 to the constant and > >> then adjusting the comparison operator: > >> > >> (unsigned long long) op < CONST + 1 > >> > >> but we cannot do that when the constant is already the largest value. > > > > Sounds like a target-independent bug in the making, lurking and > > waiting for a target to do the above adjustment but missing the > > bounds-check. > > > > The normal canonicalize_comparison operation tries to simplify > expressions by reducing the value towards zero and making the > appropriate adjustment to the comparison. Arm is a bit different due to > the way constants are encoded and also because of the limitations on how > we expand DImode in the backend. So I don't think this is quite as > general as you suggest. > > >> gcc/testsuite/gcc.dg/pr100563.c | 9 +++++++++ > >> 2 files changed, 34 insertions(+), 4 deletions(-) > >> create mode 100644 gcc/testsuite/gcc.dg/pr100563.c > > > > I'll therefore humbly suggest the test-case adjusted to be a > > run-time check (and if not done by others, projecting to do that > > myself...some time late next summer9. > > That being said, I won't object if you want to /add/ an additional test > to try to catch this; but this test had to be quite carefully crafted to > hit the specific bug in the arm expanders, so I'd prefer that it wasn't > modified directly. Note some less special crafting might be needed when using the GIMPLE FE and you start right with RTL expansion via __GIMPLE (ssa,startwith("expand")) -fdump-tree-optimized-gimple should provide rough GIMPLE FE input. Richard. > R. > > > > > brgds, H-P > >
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index 2962071adfd..d0c0c50be97 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -5563,9 +5563,20 @@ arm_canonicalize_comparison (int *code, rtx *op0, rtx *op1, return; *op1 = GEN_INT (i + 1); *code = *code == GT ? GE : LT; - return; } - break; + else + { + /* GT maxval is always false, LE maxval is always true. + We can't fold that away here as we must make a + comparison, but we can fold them to comparisons + with the same result that can be handled: + op0 GT maxval -> op0 LT minval + op0 LE maxval -> op0 GE minval + where minval = (-maxval - 1). */ + *op1 = GEN_INT (-maxval - 1); + *code = *code == GT ? LT : GE; + } + return; case GTU: case LEU: @@ -5578,9 +5589,19 @@ arm_canonicalize_comparison (int *code, rtx *op0, rtx *op1, return; *op1 = GEN_INT (i + 1); *code = *code == GTU ? GEU : LTU; - return; } - break; + else + { + /* GTU ~0 is always false, LEU ~0 is always true. + We can't fold that away here as we must make a + comparison, but we can fold them to comparisons + with the same result that can be handled: + op0 GTU ~0 -> op0 LTU 0 + op0 LEU ~0 -> op0 GEU 0. */ + *op1 = const0_rtx; + *code = *code == GTU ? LTU : GEU; + } + return; default: gcc_unreachable (); diff --git a/gcc/testsuite/gcc.dg/pr100563.c b/gcc/testsuite/gcc.dg/pr100563.c new file mode 100644 index 00000000000..812eb9e6ae2 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr100563.c @@ -0,0 +1,9 @@ +/* { dg-do compile } */ +/* { dg-options "-Og" } */ +unsigned long long e(void); +void f(int); +void a() { + short b = -1, c = (int)&b; + unsigned long long d = e(); + f(b >= d); +}