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

Fri Oct 29 06:20:08 PDT 2021

nlopes added a comment.

> - Should a regular vector add (unmasked) keep the `nsw`/`nuw` flags if one vector lane X might overflow?

Yes. The result won't be used if it wasn't used by the original program (except for widened loads/stores, as discussed before)

> - What if the result of the vector add is used by a masked vector store which masks out lane X?

No problem. Masked store is equivalent to:
if (mask[i])

  store v[i] p[i]

So v[i] can be poison because it won't be used.

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

Yep, same as the previous. But as long as the address is not used for a widened load/store.

> - What about vector instructions feeding a vector GEP feeding a gather/scatter?

That depends. Was the original program using that same value? If so, no problem. Otherwise I need a concrete example as I can't imagine one right away :)

> - What about FMF (https://reviews.llvm.org/D112117)?

The problem with floats with signaling NaNs I guess. It wouldn't be correct to execute those speculatively. But that's a totally different problem.
If it's just about FMF producing poison, then no worries and that patch doesn't seem necessary (because if something is broken, it's a much bigger problem).

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

It's totally fine to execute instructions that produce poison as long as their value is not used. If you use them in a masked_store, the masked out values are not used.
This is correct:

  if (i != 1)
    r[i] = a[i] +nsw b[i]
  =>
  tmp = a[i..i+3] +nsw [i..i+3]
  store a, r, <1,0,1,1>

Sorry, I don't know the exact syntax, but the point is that you can execute the add for i == 1 with nsw as you'll never use the result.

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

If by that you mean dropping flags from instructions whose values flow into addresses of widened load/store operations, yes! That's a good way of fixing the 1st issue.
Unless you know something about the mask. If the 1st lane of every iteration  is not masked out, you don't need to do anything as that value would be dereferenced in the original program.

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

That's great! I was worried your bug report exposes 2 separate problems. I just didn't know if the 2nd one happened in practice or not.

I'm happy to answer further questions, of course. If you have concrete examples in mind even better as it's easier to communicate I think.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111846