| Message ID | af12ed1d180793d9c01ffc042d6cc538f2da5897.camel@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [committed,P1,PR,target/94238] Don't create invalid comparisons | expand |
On Mon, Mar 23, 2020 at 06:00:06PM -0600, Jeff Law via Gcc-patches wrote: > +/* Return true if CODE is valid for comparisons of mode MODE, false > + otherwise. > + > + It is always safe to return false, even if the code was valid for the > + given mode as that will merely suppress optimizations. */ > + > +static bool > +comparison_code_valid_for_mode (enum rtx_code code, enum machine_mode mode) > +{ > + switch (code) > + { > + /* These are valid for integral, floating and vector modes. */ > + case NE: > + case EQ: > + case GE: > + case GT: > + case LE: > + case LT: > + return (INTEGRAL_MODE_P (mode) > + || FLOAT_MODE_P (mode) > + || VECTOR_MODE_P (mode)); I'm not sure I understand the VECTOR_MODE_P cases. INTEGRAL_MODE_P or FLOAT_MODE_P already do include vector modes with the corresponding element types. And if you want the {,u}{fract,accum} modes for some of these, shouldn't they apply to both scalar and vector modes thereof? So, either drop the || VECTOR_MODE_P (mode), or use || ALL_FRACT_MODE_P (mode) || ALL_ACCUM_MODE_P (mode) instead? > + > + /* These are valid for floating point modes. */ > + case LTGT: > + case UNORDERED: > + case ORDERED: > + case UNEQ: > + case UNGE: > + case UNGT: > + case UNLE: > + case UNLT: > + return FLOAT_MODE_P (mode); This will do the right thing, i.e. allow it for both floating scalar and vector modes. > + /* These are filtered out in simplify_logical_operation, but > + we check for them too as a matter of safety. They are valid > + for integral and vector modes. */ > + case GEU: > + case GTU: > + case LEU: > + case LTU: > + return INTEGRAL_MODE_P (mode) || VECTOR_MODE_P (mode); This doesn't look right. I don't know what the fract/accum modes want, but you don't want to return true for GET_MODE_CLASS (mode) == MODE_VECTOR_FLOAT, do you? Jakub
On Tue, 2020-03-24 at 10:50 +0100, Jakub Jelinek wrote: > On Mon, Mar 23, 2020 at 06:00:06PM -0600, Jeff Law via Gcc-patches wrote: > > +/* Return true if CODE is valid for comparisons of mode MODE, false > > + otherwise. > > + > > + It is always safe to return false, even if the code was valid for the > > + given mode as that will merely suppress optimizations. */ > > + > > +static bool > > +comparison_code_valid_for_mode (enum rtx_code code, enum machine_mode mode) > > +{ > > + switch (code) > > + { > > + /* These are valid for integral, floating and vector modes. */ > > + case NE: > > + case EQ: > > + case GE: > > + case GT: > > + case LE: > > + case LT: > > + return (INTEGRAL_MODE_P (mode) > > + || FLOAT_MODE_P (mode) > > + || VECTOR_MODE_P (mode)); > > I'm not sure I understand the VECTOR_MODE_P cases. > INTEGRAL_MODE_P or FLOAT_MODE_P already do include vector modes with the > corresponding element types. > And if you want the {,u}{fract,accum} modes for some of these, shouldn't > they apply to both scalar and vector modes thereof? > So, either drop the || VECTOR_MODE_P (mode), or use > > > ALL_FRACT_MODE_P (mode) || ALL_ACCUM_MODE_P (mode) > instead? It's unclear to me as well. I tried to follow rtl.def as closely as possible for that reason. rtl.def doesn't call out the fractional modes, accumulator modes, etc. It speaks strictly in terms of integral, float, vector mode classes. + /* These are filtered out in simplify_logical_operation, but > > + we check for them too as a matter of safety. They are valid > > + for integral and vector modes. */ > > + case GEU: > > + case GTU: > > + case LEU: > > + case LTU: > > + return INTEGRAL_MODE_P (mode) || VECTOR_MODE_P (mode); > > This doesn't look right. I don't know what the fract/accum modes want, but > you don't want to return true for GET_MODE_CLASS (mode) == > MODE_VECTOR_FLOAT, do you? Again, this is a direct translation from the documentation in rtl.def. I certainly don't mind changing it if there's a concrete proposal. jeff
diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 9ba58273c21..3b926703957 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,11 @@ +2020-03-23 Jeff Law <law@redhat.com> + + PR rtl-optimization/90275 + PR target/94238 + PR target/94144 + * simplify-rtx.c (comparison_code_valid_for_mode): New function. + (simplify_logical_relational_operation): Use it. + 2020-03-23 Jakub Jelinek <jakub@redhat.com> PR c++/91993 diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c index dd3d85156c3..28c2dc69ae7 100644 --- a/gcc/simplify-rtx.c +++ b/gcc/simplify-rtx.c @@ -2215,6 +2215,53 @@ mask_to_comparison (int mask) } } +/* Return true if CODE is valid for comparisons of mode MODE, false + otherwise. + + It is always safe to return false, even if the code was valid for the + given mode as that will merely suppress optimizations. */ + +static bool +comparison_code_valid_for_mode (enum rtx_code code, enum machine_mode mode) +{ + switch (code) + { + /* These are valid for integral, floating and vector modes. */ + case NE: + case EQ: + case GE: + case GT: + case LE: + case LT: + return (INTEGRAL_MODE_P (mode) + || FLOAT_MODE_P (mode) + || VECTOR_MODE_P (mode)); + + /* These are valid for floating point modes. */ + case LTGT: + case UNORDERED: + case ORDERED: + case UNEQ: + case UNGE: + case UNGT: + case UNLE: + case UNLT: + return FLOAT_MODE_P (mode); + + /* These are filtered out in simplify_logical_operation, but + we check for them too as a matter of safety. They are valid + for integral and vector modes. */ + case GEU: + case GTU: + case LEU: + case LTU: + return INTEGRAL_MODE_P (mode) || VECTOR_MODE_P (mode); + + default: + gcc_unreachable (); + } +} + /* Simplify a logical operation CODE with result mode MODE, operating on OP0 and OP1, which should be both relational operations. Return 0 if no such simplification is possible. */ @@ -2252,6 +2299,10 @@ simplify_logical_relational_operation (enum rtx_code code, machine_mode mode, code = mask_to_comparison (mask); + /* Many comparison codes are only valid for certain mode classes. */ + if (!comparison_code_valid_for_mode (code, mode)) + return 0; + op0 = XEXP (op1, 0); op1 = XEXP (op1, 1); diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index 5f079f1fca9..28adfd8b580 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,10 @@ +2020-03-23 Jeff Law <law@redhat.com> + + PR target/94144 + PR target/94238 + * gcc.c-torture/compile/pr94144.c: New test. + * gcc.c-torture/compile/pr94238.c: New test. + 2020-03-23 Patrick Palka <ppalka@redhat.com> PR c++/93805 diff --git a/gcc/testsuite/gcc.c-torture/compile/pr94144.c b/gcc/testsuite/gcc.c-torture/compile/pr94144.c new file mode 100644 index 00000000000..4358e0a7b00 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/compile/pr94144.c @@ -0,0 +1,18 @@ + +int a, b, z; +int c(int d, int e) { return d && e > 0 && d > 5 - e ? 0 : d + e; } +int k(); +void h(int); +void f(short d) { + int g = !(0 < d); + h(d); + if (b) { + unsigned i[1]; + i[0] = g = 0; + for (; g <= 8; g++) + d || k(); + if (c(!(i[0] <= z) >= d, d) != a) + k(); + } +} + diff --git a/gcc/testsuite/gcc.c-torture/compile/pr94238.c b/gcc/testsuite/gcc.c-torture/compile/pr94238.c new file mode 100644 index 00000000000..5a96a64d85b --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/compile/pr94238.c @@ -0,0 +1,22 @@ +enum { false, true } a; +int b, c, d, e, f; +int fn3(); +void fn2(); + +void fn1() { + _Bool g, h = false, i = false; + int j; + c = b && f || d; + if (c) { + if (d) + i = true; + _Bool k = b; + int l = e, m = a; + g = k && l < m || l > m; + } + if (g) + h = true; + if (i) + fn2(); + h &&j &&fn3(); +}
As outlined in the BZ simplify_logical_relational_operation does not validate that the comparison code it selects is ultimately valid for the mode of the comparison. So we could have something like: (ior (lt (...)) (gt (...)) Which it happily turns into (ltgt (...)) Which is not valid for integral modes. This patch validates the selected comparison code is valid for the comparison's mode using the rules in rtl.def. It's always safe to reject a particular simplification. I've also verified this fixes 94144 which is a similar problem showing up on aarch64 and that it doesn't cause regressions in the tester. I'm committing this to the trunk. Jeff commit fd25ae64e3ffaa95abb60e74af45031d2f5f900f Author: Jeff Law <law@redhat.com> Date: Mon Mar 23 17:55:20 2020 -0600 Verify the code used for the optimized comparison is valid for the comparison's mode. PR rtl-optimization/90275 PR target/94238 PR target/94144 * simplify-rtx.c (comparison_code_valid_for_mode): New function. (simplify_logical_relational_operation): Use it. PR target/94144 PR target/94238 * gcc.c-torture/compile/pr94144.c: New test. * gcc.c-torture/compile/pr94238.c: New test.