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

Thu Dec 19 14:40:27 PST 2019

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!

================
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
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71660