[PATCH] D65144: [InstCombine] Fold '((%x * %y) u/ %x) != %y' to '@llvm.umul.with.overflow' + overflow bit extraction

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3378
+  if (Mul)
+    Builder.SetInsertPoint(Mul);
+
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > xbolva00 wrote:
> > > Also add multi use check?
> > > 
> > > bool HasMultiUseMul = Mul && !Mul->hasOneUse();
> > > if (..)
> > > 
> > > 
> > If we insert in place of old `mul` only if `mul` had extra uses,
> > then maybe this intrinsic will still be hoisted back into that
> > other basic block because it's loop-invariant.
> > Or maybe it we always place insert in place of old `mul`
> > it will get sinked into some other only basic block where it's used.
> > 
> > I'm honestly not sure what the best solution here is,
> > we just don't have enough information to make the final
> > (the one that won't be altered by later passes) choice here.
> > Either choice that passes `-verify` is good.
> > 
> > Consistently placing it in place of the old `mul` //seemed// least arbitrary to me.
> > 
> > Maybe others have some other ideas?
> > Maybe others have some other ideas?
> 
> Err, what i meant to say is, maybe there is some consistent justification that points
> to some particular location being the correct one to insert to?
I don't think it really matters either way, but my preference here would be (for the one-use case) to insert everything together, both because that is the default behavior (what we would do if we were matching for one-use in the first place and had no special handling) and because that makes the condition here and below consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65144