[PATCH] D75801: [InstCombine] Remove known bits constant folding (WIP)

[Nikita Popov via Phabricator via llvm-commits]

nikic marked 5 inline comments as done.
nikic added inline comments.


================
Comment at: test/Transforms/InstCombine/all-bits-shift.ll:36
+; CHECK-NEXT:    store i32 [[OR_1]], i32* [[TMP0]], align 4
+; CHECK-NEXT:    ret i32 [[OR_1]]
 ;
----------------
I believe this test case was the original motivation for having this fold.

However, I thinks this should be handled by InstCombineSimplifyDemanded, which we invoke in cases where we have a reasonable expectation of either demanded bits or known bits simplifications to occur (such as an "and" root, as is the case here). SimplifyDemanded currently doesn't handle this case due to what looks like an implementation bug to me: While normally SimplifyDemanded computes known bits for instructions it doesn't handle itself, it does not do so for some instructions it only partially handles (e.g. it handles a constant shift amount, but does not compute known bits if the shift amount is not constant).


================
Comment at: test/Transforms/InstCombine/assume.ll:340
+; CHECK-NEXT:    tail call void @llvm.assume(i1 [[CMP2]])
+; CHECK-NEXT:    ret i32 0
 ;
----------------
I'm not sure we really need to do anything about this, I think it's only important that we have an assume(false) here, and SimplifyCFG will deal with the rest.

If we do want to improve on this, we could convert assume(false) into store undef (the InstCombine UB pattern) and then remove all instructions after store undef.


================
Comment at: test/Transforms/InstCombine/expensive-combines.ll:14
 ; EXPENSIVE-ON-NEXT:    [[CALL:%.*]] = call i32 @passthru(i32 0)
-; EXPENSIVE-ON-NEXT:    call void @sink(i32 0)
+; EXPENSIVE-ON-NEXT:    call void @sink(i32 [[CALL]])
 ; EXPENSIVE-ON-NEXT:    ret void
----------------
We're missing a fold to replace call with `returned` attribute to the returned argument. Known bits calculation handles the particular case where the operand is constant, but non-constant operands are not handled at all right now.


================
Comment at: test/Transforms/InstCombine/out-of-bounds-indexes.ll:10
+; CHECK-NEXT:    [[AND1:%.*]] = and i32 [[A:%.*]], 3
+; CHECK-NEXT:    tail call void @llvm.assume(i1 undef)
+; CHECK-NEXT:    ret i32 [[AND1]]
----------------
This is an improvement.


================
Comment at: test/Transforms/InstCombine/phi-shifts.ll:13
+; CHECK:       end:
+; CHECK-NEXT:    ret i64 undef
 ;
----------------
This is an improvement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75801