On Thu, 15 Oct 2015, Marc Glisse wrote: > On Thu, 15 Oct 2015, Marek Polacek wrote: > > > On Wed, Oct 14, 2015 at 08:54:11PM +0200, Marc Glisse wrote: > > > On Wed, 14 Oct 2015, Marek Polacek wrote: > > > > > > > Evidently, the X - (X / Y) * Y -> X % Y pattern can't change the > > > > signedness of X from signed to unsigned, otherwise we'd generate > > > > wrong code. (But unsigned -> signed should be fine.) > > > > > > > > Does anyone see a better fix than this? > > > > > > I was wondering about producing: > > > > > > (convert (trunc_mod @0 @1)) > > > > That's exactly what we had before r225195. > > aka https://gcc.gnu.org/ml/gcc-patches/2015-06/msg02239.html > > Maybe that patch was wrong? There are other ways to work around the fact that > operand_equal_p ignores the type of constants. One would be to use the > pattern: > (minus (convert1? @2) (convert2? (mult (trunc_div @0 @1) @1))) > and manually test operand_equal_p (@0, @2) (for constants we could even catch > one extra rare case by testing that @0 converted to type is the same as @2), > this way @0 has the right type. Another way would be to have trunc_div@2, with > type2=TREE_TYPE(@2), and get > (convert (trunc_mod (convert:type2 @0) @1)) > > Richard, when we have several occurences of @0 in a pattern, would it be hard > to add some annotation to tell the machinery which one to capture (since they > are not equivalent in the case of constants)? Or maybe by default pick the > first one that is not directly inside "convert?"?
Ok, so we have sth like 1u - (unsigned) ((1 / Y) * Y) and @0 == 1u == 1. No, it is generally impossible to tell which @0 is picked (the theory is it shouldn't matter ;)) The workaround doing the comparison manually works and is probably the easiest fix apart from unconditionally adding converts in the result. The patch in https://gcc.gnu.org/ml/gcc-patches/2015-06/msg02239.html should probably have used (convert (trunc_mod (convert @0) @1)) instead though. Maybe not, my head hurts trying to figure out what valid conversions we should allow and which type the % has to be performed in ;) To me it looks like the above is correct - use the type of the multiplication/division. A few days ago I was playing with the idea of teaching genmatch to consider constants specially for matches and add extra adjusted patterns so one can also do sth like (simplify (plus @0 (negate @0)) ...) without worrying that (negate INTEGER_CST) will not match. But for now I settled on the plan to make predicates with arguments possible and work reliably so you can write (match neg @0 (negate @0)) (match neg @0 INTEGER_CST@1 (if (wi::add (@0, @1) == 0)) (simplify (plus @0 neg (@0)) ...) similar predicates could be added for matching conversions (of constants). First of all this requires code-gen to change executing the predicate delayed (so all possible arguments have been "materialized"). Not sure when I get to that though. > > > Aren't there also problems if the conversion changes the precision? I can > > > imagine your patch ending in x % 0, with @1 non-zero but a narrowing cast. > > > > Hmm, I haven't found such a case yet. If you find something, pass > > it along ;). > > It is hard to trigger because we are missing :c on the mult. Indeed we do. > If you add it, I > expect the following to be an issue: > > #include <stdio.h> > int f(long a, long b){ > int x = (int)a; > long z = a / b * b; > int t = (int)z; > return x-t; > } > int main(){ > long a = (1L<<33)+2; > long b = 1L<<32; > printf("%d\n", f(a,b)); > return 0; > } > > > -- Richard Biener <rguent...@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
