[PATCH] D152456: SeparateConstOffsetFromGEP: Don't use SCEV

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:1240
+      if (auto *Dom =
+              findClosestMatchingDominator({LHS, RHS}, I, DominatingAdds)) {
         Instruction *NewSExt = new SExtInst(Dom, I->getType(), "", I);
----------------
nikic wrote:
> I believe the previous code would also detect cases where the add operands are swapped as the same. Could add something like
> ```
> static std::pair<Value *, Value *> createNormalizedPair(Value *A, Value *B) {
>   if (A < B)
>     return {A, B};
>   return {B, A};
> }
> ```
> and use it for the add case to preserve that behavior. Though the fact that it's apparently untested is not reassuring...
Most the code in the pass has sparse testing. I've rebased on a new test which showed this broke


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152456/new/

https://reviews.llvm.org/D152456