This is an archive of the discontinued LLVM Phabricator instance.

[VectorCombine] Use constant range info for index scalarization legality.
ClosedPublic

Authored by fhahn on May 14 2021, 3:32 AM.

Details

Summary

We can only scalarize memory accesses if we know the index is valid.

This patch adjusts canScalarizeAcceess to fall back to
computeConstantRange to check if the index is known to be valid.

Diff Detail

Event Timeline

fhahn created this revision.May 14 2021, 3:32 AM
fhahn requested review of this revision.May 14 2021, 3:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 14 2021, 3:32 AM
fhahn updated this revision to Diff 345431.May 14 2021, 6:58 AM

Also get and pass in the AC with the legacy PM.

What about an index that has been clamped with a urem() or and() ?

fhahn updated this revision to Diff 345701.May 16 2021, 4:53 AM

rebase after adding positive & negative tests with urem & and in 2f69b78a578d

fhahn updated this revision to Diff 347367.May 24 2021, 6:14 AM

rebase & ping :)

This patch looks good if we want to go with the poison-on-OOB semantics.
I think it's important to settle that first, please see my bug report: https://bugs.llvm.org/show_bug.cgi?id=50382
The backend does this very same optimization but without paying attention to whether the index is in-bounds or not. Which effectively makes extractvalue similar to a load instruction, including all the restrictions around movement and so on. Is that what we want?
We need to either fix the backend and move on with this patch (poison on OOB), or fix langref and abandon this patch (UB on OOB). Since I don't really know which frontend produces extractelement and from which code, I don't have an opinion either way, although usually UB is not a good idea.

fhahn added a comment.May 25 2021, 3:57 AM

This patch looks good if we want to go with the poison-on-OOB semantics.
I think it's important to settle that first, please see my bug report: https://bugs.llvm.org/show_bug.cgi?id=50382

I think the conclusion of the discussion at the patch to far seems to be to keep it poison. So I think we could move ahead with this patch?

nlopes accepted this revision.May 25 2021, 4:01 AM

This patch looks good if we want to go with the poison-on-OOB semantics.
I think it's important to settle that first, please see my bug report: https://bugs.llvm.org/show_bug.cgi?id=50382

I think the conclusion of the discussion at the patch to far seems to be to keep it poison. So I think we could move ahead with this patch?

SGTM!

This revision is now accepted and ready to land.May 25 2021, 4:01 AM
efriedma added inline comments.
efriedma added inline comments.Jun 30 2021, 10:47 AM
llvm/test/Transforms/VectorCombine/load-insert-store.ll
195
fhahn added inline comments.Aug 3 2021, 9:02 AM
llvm/test/Transforms/VectorCombine/load-insert-store.ll
195

Apologies for the delay! I put up a patch limiting the transform to cases where the index is known non-poison: D107364

fhahn marked 2 inline comments as done.Aug 12 2021, 3:13 AM
fhahn added inline comments.
llvm/test/Transforms/VectorCombine/load-insert-store.ll
195

Ok, looks like things look good now on the latest run https://web.ist.utl.pt/nuno.lopes/alive2/index.php?hash=295f040c6c7caf3e