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

[Diego Caballero via Phabricator via llvm-commits]

dcaballe added a comment.

> 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?

I focused on fixing only address computation cases initially but with the ongoing discussion I understood I was oversimplifying the problem and that we should be more conservative and generalize it to more valid cases. What is not clear to me is what a valid case is. Hopefully, you can help me understand. Should dropping poison-generating flags be UB-driven only or should they be semantically correct based on the definition of each poison-generating flag? A few related questions for my understanding:

- Should a regular vector add (unmasked) keep the `nsw`/`nuw` flags if one vector lane X might overflow?
- What if the result of the vector add is used by a masked vector store which masks out lane X?
- Should a GEP instruction keep the `inbounds` flag if after vectorization the computed address is actually out-of-bounds but out-of-bounds elements are masked out by the consumer masked load/store/gather/scatter?
- What about vector instructions feeding a vector GEP feeding a gather/scatter?
- What about FMF (https://reviews.llvm.org/D112117)?

I think I need an answer to these questions to really understand what is needed. Hopefully, you can help me with this. It sounds really interesting! :).

The definition of the poison-generating flags seems ambiguous for vector types. If the properties of a flag might not hold within the context of the instruction itself and for all the vector lanes, wouldn’t it be semantically incorrect to keep that flag? Keeping the flag because the user of that instruction will mask out the invalid lanes sounds concerning to me but maybe I’m wrong here.

> This means the fix should be limited to operands of load/store and for when only the 1st iteration is masked out.

If we wanted to go this way, we would have to prove that the first vector lane of at least one vector iteration is masked out, not necessarily the first lane of the first vector iteration. It seems complicated to prove that accurately. Any ideas? The guarding condition could be complex and missing just one flag would defeat the purpose of the fix. Maybe we could consider dropping all the flags in instructions involved in address computation. Would that be reasonable?

> 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.

LV bails out because the first phi node is not supported by the vectorizer. I can definitely add a test for this case.

It would be great to know what the other reviewers think about this!

Thanks,
Diego


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111846