[PATCH] D40645: [SCEV][NFC] Check NoWrap flags before lexicographical comparison of SCEVs

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: llvm/trunk/lib/Analysis/ScalarEvolution.cpp:699
+    if (LA->getNoWrapFlags() != RA->getNoWrapFlags())
+      return (int)LA->getNoWrapFlags() - (int)RA->getNoWrapFlags();
+
----------------
rtereshin wrote:
> Hi everyone,
> 
> Unfortunately, wrapping flags are not a part of SCEV's identity (they do not participate in computing a hash value or in equality comparisons) and in fact they could be assigned after the fact w/o rebuilding a SCEV.
> 
> At least, last time I checked it was that way. Grep for const_cast's to see quite a few of example, AFAIR all for AddRec's.
> 
> So, if 2 expressions get built in 2 slightly different ways: one with flags set in the beginning, the other with the flags attached later on, we may end up with 2 expressions which are exactly the same but have their operands swapped in one of the commutative N-ary expressions, and at least one of them will have "sorted by complexity" invariant broken.
> 
> 2 identical SCEV's won't compare equal by pointer comparison as they are supposed to.
> 
> I didn't verify this with an experiment yet, please correct me if I'm wrong.
> 
> Thanks,
> Roman
I think you are right, and it's a good reason to revert this patch. Please do so with checking in a test where it shows up. Good catch!


Repository:
  rL LLVM

https://reviews.llvm.org/D40645