[PATCH] D96663: [InstCombine] Fold icmp (select c, const, arg), null if arg has nonnullattr

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune marked 2 inline comments as done.
aqjune added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp:3210
+InstCombinerImpl::foldICmpInstWithConstantNotInt(ICmpInst &I,
+                                                 const SimplifyQuery &Q) {
   Value *Op0 = I.getOperand(0), *Op1 = I.getOperand(1);
----------------
nikic wrote:
> As this is already on InstCombinerImpl, is the Q argument necessary?
Removed it, thanks


================
Comment at: llvm/test/Transforms/InstCombine/assume-icmp-null-select.ll:20
+  %4 = icmp ne i8* %.0.i, null
+  call void @llvm.assume(i1 %4)
+  ret i8* %.0.i
----------------
nikic wrote:
> I find the presence of assume in this example a bit confusing, as your transform does not actually use information from the assume. I'd replace it with a more generic "use".
`llvm.assume` is necessary because it tells that `%y_is_null` is false, helping `ret i8* %res` to be transformed into `ret i8* %x`.

The series of transformations looks like this:

(1) InstCombine visits `%nonnull = icmp ne i8* %res, null`
(2) `foldICmpInstWithConstantNotInt` looks into the definition of `%res` which is `select i1 %y_is_null, i8* null, i8* %x` and folds `%nonnull` into `select i1 %y_is_null, false, true`.
During this folding, `%x`'s `dereferenceable` tag is used by ValueTracking to answer isNonNull query.
(3) The `llvm.assume` now tells that `%y_is_null` is false, allowing the `ret i8* %res` to be transformed into `ret i8* %x`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96663