Page MenuHomePhabricator

[AMDGPU] Add VReg_192/VReg_224 support for MIMG instructions
ClosedPublic

Authored by critson on Jun 7 2021, 3:40 AM.

Details

Summary

Allow MIMG instructions to be selected with 6/7 VGPRs for vaddr.
Previously these were rounded up to VReg_256 this saves VGPRs.

Diff Detail

Unit TestsFailed

TimeTest
760 msx64 debian > libomp.lock::omp_init_lock.c
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /var/lib/buildkite-agent/builds/llvm-project/openmp/runtime/test -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -I /var/lib/buildkite-agent/builds/llvm-project/openmp/runtime/test/ompt /var/lib/buildkite-agent/builds/llvm-project/openmp/runtime/test/lock/omp_init_lock.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/test/lock/Output/omp_init_lock.c.tmp -lm -latomic && /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/test/lock/Output/omp_init_lock.c.tmp

Event Timeline

critson created this revision.Jun 7 2021, 3:40 AM
critson requested review of this revision.Jun 7 2021, 3:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2021, 3:40 AM

Maybe this is stating the obvious, but you could add v6i32 and v6f32 types. @tpr did that before for v3 and v5 vectors, specifically for AMDGPU image ops. See D58901 (and a bunch of related commits you can find with git log --grep v5f32).

Maybe this is stating the obvious, but you could add v6i32 and v6f32 types. @tpr did that before for v3 and v5 vectors, specifically for AMDGPU image ops. See D58901 (and a bunch of related commits you can find with git log --grep v5f32).

I am second to that.

arsenm added a comment.Jun 7 2021, 1:35 PM

It's not that difficult to add new MVTs, although it's a bit annoying

critson updated this revision to Diff 353294.Jun 21 2021, 1:19 AM

Update based on new MVT type support.

critson retitled this revision from [AMDGPU] Add VReg_192 support for MIMG instructions to [AMDGPU] Add VReg_192/VReg_224 support for MIMG instructions.Jun 21 2021, 1:19 AM
critson edited the summary of this revision. (Show Details)
dp added inline comments.Jun 21 2021, 7:42 AM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
3455

A shorter variant would probably fit in one line:

ExpectedAddrSize >= 5 && ExpectedAddrSize <= 7
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
1938

typo

1962

ditto

foad added inline comments.Jun 21 2021, 8:28 AM
llvm/lib/Target/AMDGPU/MIMGInstructions.td
766

This is just "dw"

critson updated this revision to Diff 353531.Jun 21 2021, 7:33 PM
  • Incorporate reviewer feedback
critson marked 4 inline comments as done.Jun 21 2021, 7:37 PM
critson added inline comments.
llvm/lib/Target/AMDGPU/MIMGInstructions.td
766

A leftover from testing :-)

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
1938

Thanks!

critson updated this revision to Diff 353534.Jun 21 2021, 7:38 PM
critson marked 2 inline comments as done.
  • Address reviewer comments
foad accepted this revision.Jun 22 2021, 1:34 AM

LGTM with the caveat that I don't understand all the implementation details -- in particular anything you could do to explain or simplify the complicated expression in MIMGInstructions.td would be much appreciated!

llvm/lib/Target/AMDGPU/MIMGInstructions.td
765–766

I have never understood this bit and I understand it even less now that 6 and 7 are treated differently from 3 and 5.

This revision is now accepted and ready to land.Jun 22 2021, 1:34 AM
critson updated this revision to Diff 354215.Jun 24 2021, 5:08 AM
  • Refactor tablegen code in MIMG
critson marked an inline comment as done.Jun 24 2021, 5:09 AM
critson added inline comments.
llvm/lib/Target/AMDGPU/MIMGInstructions.td
765–766

I have rewritten this to be more explicit and hence hopefully easier to understand.
Please let me know your opinion.

foad added inline comments.Jun 24 2021, 6:17 AM
llvm/lib/Target/AMDGPU/MIMGInstructions.td
765–766

Yes I think that is better thanks. I will ponder further simplifications...

This revision was automatically updated to reflect the committed changes.
critson marked an inline comment as done.