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

[Sanjay Patel via Phabricator via llvm-commits]

spatel 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
----------------
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
      }
    }

```


================
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
 
----------------
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`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101423