[PATCH] D35376: [InstCombine] Don't violate dominance when replacing instructions

[Davide Italiano via Phabricator via llvm-commits]

davide added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:3744
+          // could break the dominance relation.
+          return nullptr;
         }
----------------
serge-sans-paille wrote:
> Ok, i understand and agree with the rational behind this fix. Would it be reasonable to check if the operand is either a constant or defined in the same block, previous to current instruction, in order to catch more candidates?
As far as I can tell the canonical way to see if two instruction are ordered is to get a local numbering via `OrderedBasicBlocks`. I'm not sure it's worth the trouble here, unless there's a super-cheap way of computing it.

Note that:
1) This always assumed constant (and not instructions) for many years but never seem to have triggered.
2) The only two cases where this triggered are clearly tests crafted by a fuzzer. Whether I agree we shouldn't crash or produce something that violates SSA, I'm not sure it's good value for money.


https://reviews.llvm.org/D35376