| Message ID | alpine.DEB.2.20.1610112312310.1815@laptop-mg.saclay.inria.fr |
|---|---|
| State | New |
| Headers | show |
On Tue, Oct 11, 2016 at 11:34 PM, Marc Glisse <marc.glisse@inria.fr> wrote: > On Tue, 11 Oct 2016, Bin Cheng wrote: > >> We missed folding (convert)(X op const) -> (convert)X op (convert)const >> for unsigned narrowing because of reason reported at >> https://gcc.gnu.org/ml/gcc/2016-07/msg00126.html >> This patch fixes the issue by adding new match&simplify pattern, it also >> adds a test case. This is the prerequisite patch for next patch adding new >> vectorization pattern. > > > Some technical comments below. I am sure Jeff and/or Richi will have more to > say on the approach. I am a bit surprised to see it as adding a new > transformation, instead of moving an old one. The "old one" would be c-family/c-common.c:shorten_binary_op. It's generally prefered to move stuff, preserving semantics. We do have a similar transform for MULT_EXPR in fold-const.c which also applies to non-constant 2nd operand (likewise for shorten_binary_op). The general issue with these "narrowings" is that it loses overflow information if the original op was carried out in a TYPE_OVERFLOW_UNDEFINED type. There is also already a bunch of similar match.pd patterns here: /* Narrowing of arithmetic and logical operations. These are conceptually similar to the transformations performed for the C/C++ front-ends by shorten_binary_op and shorten_compare. Long term we want to move all that code out of the front-ends into here. */ /* If we have a narrowing conversion of an arithmetic operation where both operands are widening conversions from the same type as the outer narrowing conversion. Then convert the innermost operands to a suitable unsigned type (to avoid introducing undefined behavior), perform the operation and convert the result to the desired type. */ (for op (plus minus) (simplify (convert (op:s (convert@2 @0) (convert@3 @1))) ... so it might be possible to amend these or at least move your pattern next to those. > +/* (convert (X op C)) -> ((convert)X op (convert)C) if it is narrowing > + conversion and both types wrap when overflow. */ > +(for op (plus minus) > + (simplify > > We used to indent by a single space in this file, but I see that other > transforms have made it in with double spacing, so I guess it doesn't > matter. > > + (convert (op @0 @1)) > + (if (INTEGRAL_TYPE_P (type) > + && TYPE_OVERFLOW_WRAPS (type) > + && TREE_CODE (@1) == INTEGER_CST > > You can write (convert (op @0 INTEGER_CST@1)) and skip this line. > > + && INTEGRAL_TYPE_P (TREE_TYPE (@0)) > + && TYPE_OVERFLOW_WRAPS (TREE_TYPE (@0)) > > This seems quite restrictive, TYPE_OVERFLOW_UNDEFINED should also be fine > for this type. I guess you are trying to avoid saturating / trapping types? Maybe he's avoiding the dropping of overflow info ... at least it warrants a comment. Richard. > + && TYPE_PRECISION (type) < TYPE_PRECISION (TREE_TYPE (@0))) > + (op (convert @0) (convert @1))))) > + > /* If we have a narrowing conversion to an integral type that is fed by a > BIT_AND_EXPR, we might be able to remove the BIT_AND_EXPR if it merely > masks off bits outside the final type (and nothing else). */ > diff --git a/gcc/testsuite/gcc.dg/fold-narrow-unsgn-opcst.c > b/gcc/testsuite/gcc.dg/fold-narrow-unsgn-opcst.c > new file mode 100644 > index 0000000..aff96a9 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/fold-narrow-unsgn-opcst.c > @@ -0,0 +1,8 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O1 -fdump-tree-gimple" } */ > + > +unsigned char foo (unsigned short s) > +{ > + return (unsigned char)(s + 65530); > +} > +/* { dg-final { scan-tree-dump-not " 65530" "gimple" } } */ > > As I understand it, C says that s is promoted to int and added to 65530, but > int is not TYPE_OVERFLOW_WRAPS so your transformation doesn't apply (the > test already passes without your patch). It is better to write tests for the > gimple version of transformations, i.e. don't write everything as a single > expression, use intermediate variables. > > -- > Marc Glisse
On 10/12/2016 02:48 AM, Richard Biener wrote: > On Tue, Oct 11, 2016 at 11:34 PM, Marc Glisse <marc.glisse@inria.fr> wrote: >> On Tue, 11 Oct 2016, Bin Cheng wrote: >> >>> We missed folding (convert)(X op const) -> (convert)X op (convert)const >>> for unsigned narrowing because of reason reported at >>> https://gcc.gnu.org/ml/gcc/2016-07/msg00126.html >>> This patch fixes the issue by adding new match&simplify pattern, it also >>> adds a test case. This is the prerequisite patch for next patch adding new >>> vectorization pattern. >> >> >> Some technical comments below. I am sure Jeff and/or Richi will have more to >> say on the approach. I am a bit surprised to see it as adding a new >> transformation, instead of moving an old one. > > The "old one" would be c-family/c-common.c:shorten_binary_op. It's generally > prefered to move stuff, preserving semantics. Right. Kai and I hadn't looked much at shorten_binary_op (focusing more on shorten_compare). But the same principles apply to both. Namely that the existing routines should be twiddled to handle warnings only, but not modify the underlying IL. IL modifications (canonicalization and optimization) should be moved into the match.pd framework. When Kai left Red Hat, that work stalled. I've got bits and pieces of that work lying around, but I don't think they'd help Bin's work right now. > > There is also already a bunch of similar match.pd patterns here: [ ... ] Right. Those were a first start at handling some of the desired narrowing, focused primarily on BZs that required narrowing to resolve. Like Kai's work, I have some generalizations and improvements in a half-completed state here, but haven't had time to work on them. Jeff
On Thu, Oct 13, 2016 at 5:22 PM, Jeff Law <law@redhat.com> wrote: > On 10/12/2016 02:48 AM, Richard Biener wrote: >> >> On Tue, Oct 11, 2016 at 11:34 PM, Marc Glisse <marc.glisse@inria.fr> >> wrote: >>> >>> On Tue, 11 Oct 2016, Bin Cheng wrote: >>> >>>> We missed folding (convert)(X op const) -> (convert)X op (convert)const >>>> for unsigned narrowing because of reason reported at >>>> https://gcc.gnu.org/ml/gcc/2016-07/msg00126.html >>>> This patch fixes the issue by adding new match&simplify pattern, it also >>>> adds a test case. This is the prerequisite patch for next patch adding >>>> new >>>> vectorization pattern. >>> >>> >>> >>> Some technical comments below. I am sure Jeff and/or Richi will have more >>> to >>> say on the approach. I am a bit surprised to see it as adding a new >>> transformation, instead of moving an old one. >> >> >> The "old one" would be c-family/c-common.c:shorten_binary_op. It's >> generally >> prefered to move stuff, preserving semantics. > > Right. Kai and I hadn't looked much at shorten_binary_op (focusing more on > shorten_compare). But the same principles apply to both. > > Namely that the existing routines should be twiddled to handle warnings > only, but not modify the underlying IL. IL modifications (canonicalization > and optimization) should be moved into the match.pd framework. fortunately shorten_binary_op itself does not emit any warnings! > When Kai left Red Hat, that work stalled. I've got bits and pieces of that > work lying around, but I don't think they'd help Bin's work right now. > >> >> There is also already a bunch of similar match.pd patterns here: > > [ ... ] > Right. Those were a first start at handling some of the desired narrowing, > focused primarily on BZs that required narrowing to resolve. Like Kai's > work, I have some generalizations and improvements in a half-completed state > here, but haven't had time to work on them. > > > Jeff
diff --git a/gcc/testsuite/gcc.dg/fold-narrow-unsgn-opcst.c b/gcc/testsuite/gcc.dg/fold-narrow-unsgn-opcst.c new file mode 100644 index 0000000..aff96a9 --- /dev/null +++ b/gcc/testsuite/gcc.dg/fold-narrow-unsgn-opcst.c @@ -0,0 +1,8 @@ +/* { dg-do compile } */ +/* { dg-options "-O1 -fdump-tree-gimple" } */ + +unsigned char foo (unsigned short s) +{ + return (unsigned char)(s + 65530); +} +/* { dg-final { scan-tree-dump-not " 65530" "gimple" } } */