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

[serge via Phabricator via llvm-commits]

serge-sans-paille added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:3744
+          // could break the dominance relation.
+          return nullptr;
         }
----------------
davide wrote:
> davide wrote:
> > 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.
> Sigh, what I mean here is: "the canonical way to check if an instruction is defined before or after another in the same BB"
Argument 1. looks 100% valid to me!


https://reviews.llvm.org/D35376