[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:3347
                          m_Value(Y)))) {
+    Mul = nullptr;
     // Canonicalize as-if %y was on RHS.
----------------
xbolva00 wrote:
> ..remove
Why i believe this is the conceptually-wrong solution:
Consider this:
```
void *data;
bool something = false;
if(some_check(&data, &something))
else if(another_check(&data))
else return;

if(something)
  ...
```

The problem is that while this may seem ok, we did zero-init `something` initially after all,
we then did pass it to `some_check()`, and it may have changed it's value.
Now, if `some_check()` returned `false`, we do `another_check()`,
but there we don't do anything with `something`.
We might intended to keep whatever we got there from `some_check()`,
or maybe we did not and simply forgot to re-zero-init it.
And in latter case `something` is essentially undefined, and we have a bug that won't be fun to debug.

Compare with:
```
void *data;
bool something; // if we do not init it before using MSAN will catch it.
if(some_check(&data, &something))
else if(another_check(&data))
  something = false; // yay
else return;

if(something) // all certainly good here.
  ...
```
Does this make sense?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65144