[PATCH] D111846: [LV] Drop integer poison-generating flags from instructions that need predication

[Nuno Lopes via Phabricator via llvm-commits]

nlopes added a comment.

In D111846#3092783 <https://reviews.llvm.org/D111846#3092783>, @dcaballe wrote:

>> The second point is that it seems the patch drops attributes from all hoisted instructions, but that's not strictly needed. You only need to drop attributes from instructions that contribute to instructions that are widened and that produce UB if given poison. I don't think LLVM can produce e.g. a division of a vector by a scalar (if the loop always divides by the same value).
>
> LV can produce a division of a vector by a scalar by broadcasting the scalar into a vector. Is that what you mean?

My point was that load/store are special in how they are widened. A vector load of %p is equivalent of load %p, load %p+1, load %p+2, etc.
So internally load produces the operands that would have been executed in subsequent iterations.
The root of the problem being addressed here is that we only pass the first operand and then have the subsequent computed internally by the operation. If we hit the case when the 1st iteration is the one masked out we may be in trouble.
It's a pretty cool bug! :)

Two conclusions from this:

- If the 1st iteration (of the vectorized bundle) isn't masked out, we are good: no need to drop any attributes. This is because we would hit any UB in the original program anyway.
- We only need to drop attributes from instructions that flow into the operands of instructions that have the property of internally computing the operands for subsequent sub-operations, like in load/store that increment pointers. I don't think LLVM has any other operation other than load/store with this property?

This means the fix should be limited to operands of load/store and for when only the 1st iteration is masked out.
Right now it seems that the code will drop attributes from any instruction regardless of whether it flows into a load/store or not. It's way too conservative. I would rather see it fixed properly now than get a promise of a fix in the future (that statistically rarely happen in the LLVM community).

To make things fully correct, there's also the concern that we need to ensure the value of the 1st iteration isn't itself already poison. This is a tricky dance with SCEV and I don't know if the example I posted previously would kick in right now, but could in the future, so at least we could add that as a unit test to make sure we don't regress if SCEV becomes smarter.

> I think the key points here are the instructions with the attributes and their guarding predicate. These instructions, *regardless of their uses*, may produce a poison value and UB themselves if the guarding predicate is dropped as a result of vectorization because they will be executed for iterations of the loop that wouldn't be executed in the original scalar loop.

It's totally fine to execute instructions that yield poison. They won't lead to UB unless used.
Instructions that may produce UB themselves can't be hoisted unless predicated, but I hope that's already accounted for.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111846