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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3378
+  if (Mul)
+    Builder.SetInsertPoint(Mul);
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65144