[PATCH] D149404: [ValueTracking][InstCombine] Add a new API to allow to ignore poison generating flags or metadatas when implying poison

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:6302
+impliesPoison(Value *ValAssumedPoison, const Value *V, unsigned Depth,
+              SmallVectorImpl<Instruction *> *IgnoredInsts = nullptr) {
   if (isGuaranteedNotToBeUndefOrPoison(ValAssumedPoison))
----------------
Hm, maybe just directly expose the API with the optional arg? Not sure we need one without the arg and one with it required... No strong opinion though.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2938
+    SmallVector<Instruction *> DropFlags;
+    if (impliesPoisonIgnoreFlagsOrMetadatas(FalseVal, CondVal, DropFlags)) {
+      for (Instruction *I : DropFlags)
----------------
goldstein.w.n wrote:
> Will you ever not loop through instruction and do `dropPoisonGenerating...`? Maybe it should done by `impliesPoisonIgnore...` and if its not always with a `bool` to control it.
If it's part of a larger transform, one may only want to drop the flags after additional conditions have been checked as well. Analysis utils generally aren't allowed to modify instructions.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2943
+      return BinaryOperator::CreateOr(CondVal, FalseVal);
+    }
+
----------------
Why did this code get moved?


================
Comment at: llvm/test/Transforms/InstCombine/ispow2.ll:917
 ; CHECK-LABEL: @is_pow2or0_ctpop_wrong_pred2_logical(
-; CHECK-NEXT:    ret i1 true
+; CHECK-NEXT:    [[T0:%.*]] = tail call i32 @llvm.ctpop.i32(i32 [[X:%.*]]), !range [[RNG0]]
+; CHECK-NEXT:    [[CMP:%.*]] = icmp ne i32 [[T0]], 1
----------------
goldstein.w.n wrote:
> StephenFan wrote:
> > I think this regression can be fixed If InstCombine matches this pattern. 
> What changed in the impl that caused this?
I expect this got handled by the select value equivalence fold before. It's okay to regress for now because it was enabled as a side-effect of  https://github.com/llvm/llvm-project/commit/bf23b4031eeabfccd46a25ce68414d45ae761304, and this patch basically undoes the effects of that one on this file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149404