[PATCH] D149893: Rewrite LSV to handle longer chains.

Thu May 4 12:40:17 PDT 2023

jlebar created this revision.
Herald added subscribers: kosarev, mattd, gchakrabarti, asavonic, kerbowa, pengfei, hiraditya, tpr, jvesely.
Herald added a project: All.
jlebar requested review of this revision.
Herald added subscribers: llvm-commits, pcwang-thead, jholewinski.
Herald added a project: LLVM.

The motivation for this change is a workload generated by the XLA compiler
targeting nvidia GPUs.  This kernel has a few hundred i8 loads and stores.
Merging is critical for performance.

The current load-store-vectorizer doesn't merge these well because it only
considers instructions within a block of 64 loads+stores.  This limit is
necessary to contain the O(n^2) behavior of the pass.  I'm hesitant to increase
the limit, because this pass is already one of the slowest parts of compiling
an XLA program.

So we rewrite basically the whole thing to use a new algorithm.

Before, we compared every load/store to every other to see if they're
consecutive.  The insight (from tra@) is that this is redundant.  If we know
the offset from PtrA to PtrB, then we don't need to compare PtrC to both of
them in order to tell whether C may be adjacent to A or B.

So that's what we do.  When scanning a basic block, we maintain a list of
chains, where we know the offset from every element in the chain to the first
element in the chain.  Each instruction gets compared only to the leaders of
all the chains.

In the worst case, this is still O(n^2), because all chains might be of length

1. To prevent compile time blowup, we only consider the 64 most recently used

chains.  Thus we do no more comparisons than before, but we have the potential
to make much longer chains.

I also hope the algorithm of the new pass is also easier to understand.  The
old pass mixed together the logic for splitting chains according to various
failure modes: may-alias instrs, legalization, alignment, etc.  The new pass
separates these concerns.  The way it splits chains is also simpler to
understand, and it also often is able to vectorize more, independent of the
chain-length improvements.

This rewrite affects many tests.  The changes to tests fall into three
categories.

1. Now we only generate vectors of power-of-two size.  Previously the code was inconsistent about whether this was a requirement.

2. The old code had what appears to be a bug when deciding whether a misaligned vectorized load is fast.  Suppose TTI reports that load <i32 x 4> align 4 has relative speed 1, and suppose that load i32 align 4 has relative speed 32.

  The intent of the code seems to be that we prefer the scalar load, because it's faster.  But the old code would choose the vectorized load. accessIsMisaligned would set RelativeSpeed to 0 for the scalar load (and not even call into TTI to get the relative speed), because the scalar load is aligned.

  After this patch, we will prefer the scalar load if it's faster.

3. This patch changes the logic for how we vectorize.  Usually this results in vectorizing more.

Explanation of changes to tests:

- AMDGPU/adjust-alloca-alignment.ll: #2
- AMDGPU/flat_atomic.ll: #3, we vectorize more.
- AMDGPU/int_sideeffect.ll: #3, there are two possible locations for the call to @foo, and the pass is brittle to this.  Before, we'd vectorize in case 1 and not case 2.  Now we vectorize in case 2 and not case 1.  So we just move the call.
- AMDGPU/adjust-alloca-alignment.ll: #3, we vectorize more
- AMDGPU/insertion-point.ll: #1 (insert_store_point_alias), and #3 we vectorize more (insert_store_point_alias_ooo)
- AMDGPU/merge-stores.ll: #1
- AMDGPU/merge-stores-private.ll: #2 (undoes changes from git rev 86f9117d476 <https://reviews.llvm.org/rG86f9117d476bcef2f5e0eabae4781e99877ce7b5>, which appear to have hit the bug from #2)
- AMDGPU/multiple_tails.ll: #2
- AMDGPU/select-insertpoison.ll: #1
- AMDGPU/selects.ll: #1
- AMDGPU/vect-ptr-ptr-size-mismatch.ll: Fix alignment (I think related to #2 above).
- NVPTX/4x2xhalf.ll: Fix alignment (I think related to #2 above).
- NVPTX/vectorize_i8.ll: #1
- X86/correct-order.ll: #3, we vectorize more, probably because of changes to the chain-splitting logic.
- X86/subchain-interleaved.ll: #3, we vectorize more
- X86/vector-scalar.ll: #3, we can now vectorize scalar float + <1 x float>
- X86/vectorize-i8-nested-add-inseltpoison.ll: Deleted the nuw test because it was nonsensical.  It was doing `add nuw %v0, -1`, but this is equivalent to `add nuw %v0, 0xffff'ffff`, which is equivalent to asserting that %v0 == 0.
- X86/vectorize-i8-nested-add.ll: Same as nested-add-inseltpoison.ll

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D149893

Files:
  llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp
  llvm/test/Transforms/InferAddressSpaces/AMDGPU/flat_atomic.ll
  llvm/test/Transforms/LoadStoreVectorizer/AMDGPU/adjust-alloca-alignment.ll
  llvm/test/Transforms/LoadStoreVectorizer/AMDGPU/insertion-point.ll
  llvm/test/Transforms/LoadStoreVectorizer/AMDGPU/merge-stores-private.ll
  llvm/test/Transforms/LoadStoreVectorizer/AMDGPU/merge-stores.ll
  llvm/test/Transforms/LoadStoreVectorizer/AMDGPU/multiple_tails.ll
  llvm/test/Transforms/LoadStoreVectorizer/AMDGPU/selects-inseltpoison.ll
  llvm/test/Transforms/LoadStoreVectorizer/AMDGPU/selects.ll
  llvm/test/Transforms/LoadStoreVectorizer/AMDGPU/vect-ptr-ptr-size-mismatch.ll
  llvm/test/Transforms/LoadStoreVectorizer/NVPTX/4x2xhalf.ll
  llvm/test/Transforms/LoadStoreVectorizer/NVPTX/many_loads_stores.ll
  llvm/test/Transforms/LoadStoreVectorizer/NVPTX/overlapping_chains.ll
  llvm/test/Transforms/LoadStoreVectorizer/NVPTX/vectorize_i16.ll
  llvm/test/Transforms/LoadStoreVectorizer/NVPTX/vectorize_i24.ll
  llvm/test/Transforms/LoadStoreVectorizer/NVPTX/vectorize_i8.ll
  llvm/test/Transforms/LoadStoreVectorizer/NVPTX/vectorize_vectors.ll
  llvm/test/Transforms/LoadStoreVectorizer/X86/correct-order.ll
  llvm/test/Transforms/LoadStoreVectorizer/X86/subchain-interleaved.ll
  llvm/test/Transforms/LoadStoreVectorizer/X86/vector-scalar.ll
  llvm/test/Transforms/LoadStoreVectorizer/X86/vectorize-i8-nested-add-inseltpoison.ll
  llvm/test/Transforms/LoadStoreVectorizer/X86/vectorize-i8-nested-add.ll
  llvm/test/Transforms/LoadStoreVectorizer/int_sideeffect.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D149893.519612.patch
Type: text/x-patch
Size: 195637 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230504/6c1784ad/attachment-0001.bin>