[PATCH] D93065: [InstCombine] Disable optimizations of select instructions that causes propagation of poison values

Wed Feb 3 08:00:29 PST 2021

aqjune added a comment.

Hi,

In D93065#2525779 <https://reviews.llvm.org/D93065#2525779>, @nikic wrote:

> @aqjune Is https://bugs.llvm.org/show_bug.cgi?id=48353 the only known (real-life) issue, or are there others? I'm okay with making shift optimization temporarily more conservative again. Not sure to what degree this addresses the issue though, even as as workaround. Shouldn't it also be easy to trigger something based on `add nsw` arithmetic in C? Or is there a reason why that is less likely?

There is a reason why other ops are less likely to be problematic - existing transformations don't simplify non-shifts with well-defined operands into undef or poison.
For example, `add nuw -1, -1` isn't being folded to poison. Similarly `gep inbounds` as well even if the offset is proven to be out of bounds (unless it is undef).
Vector operations like `extractelement` may produce poison from well-defined operands, but I believe the introduction of such instruction itself rarely happens.
The existence of such folding matters because a well-defined program that is emitted from the frontend is unlikely to have undef or poison as operands of arithmetic operations; it must be created from somewhere else, such as from `1u << 33`.

But, since there can exist an unexpected situation that I'm not aware of, I reverted D92270 <https://reviews.llvm.org/D92270> . I think the InstCombine shift patch (D93998 <https://reviews.llvm.org/D93998>) should be reverted as well.

> A thought I had is to do something like https://gist.github.com/nikic/e855476bcd87124ff8550ad9b5432f26, basically assume that clang has annotated everything possible as noundef, which should give us a pretty good heuristic to distinguish "poison might practically occur here" and "poison can theoretically occur here". The main casualty of that approach is the "one hot merge" optimization, which just doesn't seem valid to do with selects.

I agree this approach will be helpful.
Seeing arguments as no-undef is practically fine. This simulates the alternative semantics which is that passing undef or poison as a function argument is UB. (strictly speaking, this makes dead arg elim/function outlining/hoisting fncall invalid, but for analysis purpose I think this won't be a source of miscompilation)
For loads, seeing them as noundef might be allowing too much I think; they are a source of undefs from reading uninitialized values. Maybe we can start with inferring and attaching `!noundef` to loads first.

I'll be able to restart working on things after a week or earlier.

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

https://reviews.llvm.org/D93065