[PATCH] D102966: [CVP] Guard against poison in common phji value transform (PR50399)

Mon May 24 09:45:25 PDT 2021

nikic added inline comments.

================
Comment at: llvm/test/Transforms/CorrelatedValuePropagation/phi-common-val.ll:142
 entry:
   %sub = sub nsw i32 0, %arg
   %cmp = icmp eq i32 %arg, -2147483648
----------------
spatel wrote:
> nikic wrote:
> > spatel wrote:
> > > Check my understanding (add this test?): if the `sub` does not have `nsw`, then we still allow this transform because the plain `sub` can not create poison and `arg` is not poison?
> > Yes, that's right. I've added the additional test in https://github.com/llvm/llvm-project/commit/e42636d3c1a41a9b7c5d8095ae5ef6682e26d4a2 and it does still fold for the reason you described.
> Thanks. So it's one of those awkward cases where improving wrapping analysis could actually inhibit an optimization. I don't know if we have any plans for adjusting for that sort of thing, but might be worth a code or test comment?
Right. Unfortunately I don't have a good idea for how to retain this optimization. What the similar select optimization in InstCombine does is this monstrosity: https://github.com/llvm/llvm-project/blob/694068d0db4378d9956fe0d3234ee0c4d353d94c/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp#L1175-L1213 We unset the nowrap flags, rerun the analysis and then either do the fold or restore the flags. I don't think something like that is feasible (or advisable) here, because it will interact with the LVI caching in a dangerous way.

That said, I don't think this is a particularly important corner case either.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102966