This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Cluster MIMG instructions
ClosedPublic

Authored by foad on Feb 5 2020, 3:13 AM.

Diff Detail

Event Timeline

foad created this revision.Feb 5 2020, 3:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2020, 3:13 AM
foad added a subscriber: rtaylor.Feb 5 2020, 3:14 AM

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

This absolutely needs a test with sampled images.

More generally, I'm worried about whether the generic infrastructure will do something useful with the extra address arguments, so this needs quite a bit of test coverage about what happens with different texture coordinate patterns.

foad updated this revision to Diff 265457.May 21 2020, 1:52 AM

Add an image_sample test.

foad added a comment.May 21 2020, 2:17 AM

This absolutely needs a test with sampled images.

I've added one.

More generally, I'm worried about whether the generic infrastructure will do something useful with the extra address arguments, so this needs quite a bit of test coverage about what happens with different texture coordinate patterns.

The only code that knows about the address operands is SIInstrInfo::getMemOperandsWithOffset (which produces a list of address operands) and SIInstrInfo::shouldClusterMemOps (which consumes it). The logic is currently very simple: cluster if the first address operand (the image descriptor) is the same.

I'm sure the logic could be much cleverer, but I'd like to leave that for later patches with their own test cases. Until that happens, I'm not sure that it's worth producing tests for different texture coordinate patterns.

arsenm added inline comments.May 21 2020, 7:37 AM
llvm/test/CodeGen/AMDGPU/cluster_stores.ll
0–2

This could use a temp file to avoid compiling this twice. Redirect stderr to a temp file and run the dbg check on that?

92

Could use instruction checks that these are adjacent?

foad updated this revision to Diff 268141.Jun 3 2020, 4:37 AM

Use a temporary file.
Check instructions are adjacent in the ISA.

foad marked 2 inline comments as done.Jun 3 2020, 4:37 AM
arsenm accepted this revision.Jun 8 2020, 5:10 AM
arsenm added inline comments.
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
2763

Braces

This revision is now accepted and ready to land.Jun 8 2020, 5:10 AM
foad marked an inline comment as done.Jun 8 2020, 6:02 AM
This revision was automatically updated to reflect the committed changes.