This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Add offsets to MMO when lowering buffer intrinsics
ClosedPublic

Authored by tstellar on Jul 22 2019, 9:20 AM.

Details

Summary

Without offsets on the MachineMemOperands (MMOs),
MachineInstr::mayAlias() will return true for all reads and writes to the
same resource descriptor. This leads to O(N^2) complexity in the MachineScheduler
when analyzing dependencies of buffer loads and stores. It also limits
the SILoadStoreOptimizer from merging more instructions.

This patch reduces the compile time of one pathological compute shader
from 12 seconds to 1 second.

Event Timeline

tstellar created this revision.Jul 22 2019, 9:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2019, 9:20 AM

We could do the same for the raw and struct variants of the intrinsics, but I wasn't sure exactly what conditions had to be met in order to guarantee that we had a constant offset.

Can you make the test name more specific for buffer intrinsics

tstellar updated this revision to Diff 211151.Jul 22 2019, 11:02 AM

Rename test.

arsenm accepted this revision.Jul 24 2019, 8:40 AM

LGTM> I think this works as we don't have any other way to index into buffers

This revision is now accepted and ready to land.Jul 24 2019, 8:40 AM
arsenm requested changes to this revision.Jul 24 2019, 10:58 AM
arsenm added inline comments.
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
6268–6271

Why aren't these handled?

6944–6948

Why aren't these handled?

This revision now requires changes to proceed.Jul 24 2019, 10:58 AM
tstellar marked an inline comment as done.Jul 24 2019, 4:10 PM
tstellar added inline comments.
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
6944–6948

I didn't handle the raw or struct intrinsics, because I wasn't sure if the swizzling or something else from the resource descriptor affected the offset value.

arsenm added inline comments.Jul 24 2019, 5:06 PM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
6944–6948

I think the point of splitBufferOffsets is to abstract this, so it should be the same. I think the non struct/raw versions are deprecated

tstellar updated this revision to Diff 212720.Jul 31 2019, 9:19 PM

Add support for the raw and struct intrinsics.

tstellar marked an inline comment as done.Jul 31 2019, 9:22 PM
arsenm added inline comments.Jul 31 2019, 9:28 PM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
6072

Should get a comment

6073–6074

No references needed

6075

No pointer needed, VIndex = SDValue()

tstellar updated this revision to Diff 213938.Aug 7 2019, 10:12 AM

Update offsets for the raw and struct intrinsic variants.

tstellar updated this revision to Diff 213944.Aug 7 2019, 10:23 AM
tstellar marked 3 inline comments as done.

Address review comments.

arsenm accepted this revision.Oct 7 2019, 1:05 PM

LGTM

This revision is now accepted and ready to land.Oct 7 2019, 1:05 PM
This revision was automatically updated to reflect the committed changes.