[PATCH] D101423: [InstCombine] Fold overflow bit of [u|s]mul.with.overflow in a poison-safe way

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2587
 }
 
+/// The @llvm.[us]mul.with.overflow intrinsic could have been folded from some
----------------
spatel wrote:
> aqjune wrote:
> > The function below is based on InstSimplify's version.
> Duplicating so much code feels wrong. Could we just call the simplify code from here with something like:
> 
> ```
>     // Simulate a bitwise logic op for a select with a constant operand. 
>     if (match(FalseVal, m_Zero())) {
>       if (Value *Simplified =
>               SimplifyAndInst(CondVal, TrueVal, SQ.getWithInstruction(&SI))) {
>         // create freeze and return
>       }
>     }
>     if (match(TrueVal, m_One())) {
>       if (Value *Simplified =
>               SimplifyOrInst(CondVal, FalseVal, SQ.getWithInstruction(&SI))) {
>         // create freeze and return
>       }
>     }
> 
> ```
I think we still need to look into the expression because we should determine which variable to freeze.

If there is a header file that InstSimplify and InstCombine can share would be a great place for this function?


================
Comment at: llvm/test/Transforms/PhaseOrdering/unsigned-multiply-overflow-check.ll:8
 ; RUN: opt -instcombine -simplifycfg -simplifycfg-require-and-preserve-domtree=1 -instcombine -phi-node-folding-threshold=3 -S < %s | FileCheck %s --check-prefix=INSTCOMBINESIMPLIFYCFGINSTCOMBINE
 ; RUN: opt -instcombine -simplifycfg -simplifycfg-require-and-preserve-domtree=1 -instcombine -instcombine-unsafe-select-transform=0 -phi-node-folding-threshold=3 -S < %s | FileCheck %s --check-prefix=INSTCOMBINESIMPLIFYCFGINSTCOMBINEUNSAFE
 
----------------
spatel wrote:
> aqjune wrote:
> > `-instcombine-unsafe-select-transform=0` is added to show the impact of this patch.
> I would prefer to leave this file unchanged with this patch, so we still have an indicator to represent the overall behavior of the optimizer.
> 
> We should add minimal tests for this patch under test/Transforms/InstCombine. These could be based on the test files from D65150 / D65151 - just replace the and/or logic instructions with `select`?
No problem, I'll make tests like those in the next iteration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101423