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

Thu Dec 19 15:03:23 PST 2019

nikic added inline comments.

================
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:
> > 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.
If the instruction is manually sunk, then we (probably) first convert the first assume into !nonnull (because there is still the other use in the second icmp, so not ephemeral), then fold the icmp, then dce the load. Not ideal, but probably also not really preventable.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71660