[PATCH] D71660: [ValueTracking] isKnownNonZero() should take non-null-ness assumptions into consideration (PR43267)

Thu Dec 19 14:49:57 PST 2019

lebedev.ri added a comment.

Thank you for the review.

================
Comment at: llvm/test/Transforms/InstCombine/assume.ll:272
 ; CHECK:       taken:
+; CHECK-NEXT:    [[LOAD:%.*]] = load i32*, i32** [[A:%.*]], align 8
 ; CHECK-NEXT:    [[CMP:%.*]] = icmp ne i32* [[LOAD]], null
----------------
nikic wrote:
> lebedev.ri wrote:
> > nikic wrote:
> > > I'm assuming that we first evaluate the icmp below, then the load is one-use and gets sunk. I'm wondering why the assume is not converted into `!nonnull` metadata after that, as the control dependence is now gone.
> > The best i can tell is because `isEphemeralValueOf()` says so?
> > I'm not really familiar with that, best to file a bug.
> Oooh, that makes sense... If we convert the assume to !nonnull, we'd just end up eliminating the load completely afterwards and lose the information.
> 
> It might make sense to slightly tweak the test (maybe add a use of `%load` in `not_taken`?) so the sinking does not happen. As it is right now, the test no longer tests what it was originally testing, even if the transform still fails for an unrelated reason.
The confusing thing is, if i manually sink load+cmp, then we add `!nonnull` and remove everything here.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71660