[PATCH] D78615: [ValueTracking] Let propagatesPoison support binops/unaryops/cast/etc.

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune marked an inline comment as done.
aqjune added a comment.

Hello all,
I ran experiment with an alternative semantics which defines `and poison, 0` and `or poison, -1` to block propagation of poison. In this semantics, `and poison, 0` is 0, and `or poison, -1` is -1.

Here are a few optimizations that I discovered from llvm/test/Transforms becoming incorrect in the alternative semantics:

1. <Transforms/InstCombine/and-or-icmps.ll>

  define i1 @PR2330(i32 %a, i32 %b) {
  %0:
    %cmp1 = icmp ult i32 %a, 8
    %cmp2 = icmp ult i32 %b, 8
    %and = and i1 %cmp2, %cmp1
    ret i1 %and
  }
  =>
  define i1 @PR2330(i32 %a, i32 %b) {
  %0:
    %1 = or i32 %b, %a
    %2 = icmp ult i32 %1, 8
    ret i1 %2
  }

If %a = poison && %b = 9, the source returns poison && 0 = 0, whereas target returns poison.

2. <Transforms/InstCombine/apint-shift-simplify.ll>

  define i41 @test0(i41 %A, i41 %B, i41 %C) {
  %0:
    %X = shl i41 %A, %C
    %Y = shl i41 %B, %C
    %Z = and i41 %X, %Y
    ret i41 %Z
  }
  =>
  define i41 @test0(i41 %A, i41 %B, i41 %C) {
  %0:
    %X1 = and i41 %A, %B
    %Z = shl i41 %X1, %C
    ret i41 %Z
  }

If %A = 100...00 && %B = poison && %C = 1, the source returns 0 whereas target returns poison.

3. <Transforms/InstCombine/and-fcmp.ll>

  define i1 @PR1738(double %x, double %y) {
  %0:
    %cmp1 = fcmp ord %x, 0.000000
    %cmp2 = fcmp ord %y, 0.000000
    %and = and i1 %cmp1, %cmp2
    ret i1 %and 
  }
  =>
  define i1 @PR1738(double %x, double %y) {
  %0:
    %1 = fcmp ord %x, %y
    ret i1 %1
  }

If %x = poison && %y = NaN, the source returns 0 whereas target returns poison.

When I run Alive2, I see that 50 more lit tests from test/Transforms/ are failing, compared with the base semantics (and/ors unconditionally propagating poison)
To fix this - we need to redefine the semantics of many existing operations. For example, `fcmp ord` should be redefined to determine when it returns poison / when it does not, which might be a pain. Same for shl/icmp etc. So I vote for the simpler semantics (and/or unconditionally propagating poison).

Also, LangRef states that values other than select/phi depends on their operands, and an instruction that depends on a poison value, produces a poison value, so it implies that and/or propagates poison.
`propagatesPoison` in this patch returns true on and/or operations for this reason.



================
Comment at: llvm/test/Analysis/ScalarEvolution/nsw.ll:242
   ret void
 }
 
----------------
I investigated a bit, and the history was like this:

https://github.com/llvm/llvm-project/commit/7e4a64167d4d2e7b0b680fae1706182223047af1 fixed a bug in SCEV which was incorrectly adding no-wrap flag to a post-inc add recurrence. The patch added `isAddRecNeverPoison`, which checks whether it is UB if the given add recurrence is poison, by consecutively calling `propagatesFullPoison` on its use chain.

However, the patch wasn't calling `propagatesFullPoison` on the direct uses of the add recurrence, so https://github.com/llvm/llvm-project/commit/a19edc4d15b0dae0210b90615775edd76f021008 added the call & added these two tests (bad_postinc_nsw_a/b).

In bad_postinc_nsw_b, `and %iv.inc, 0` was inserted to test whether the first `propagateFullPoison` check successfully blocks propagation of poison. Now `propagatesPoison` returns true, so the test has changed.
To summarize, the validity of this change depends on whether `and x, 0` should propagate poison or not. I think it should propagate (as I wrote on another comment). As suggested, I'll change the function name & leave a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78615