Page MenuHomePhabricator

[AMDGPU][GlobalISel] Combine shift + logic + shift with constant operands

Authored by mbrkusanin on Oct 27 2020, 4:41 AM.



This sequence of instructions can be simplified if they are single use and
some operands are constants. Additional combines may be applied afterwards.

Diff Detail

Unit TestsFailed

390 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp
320 mswindows > lld.ELF/invalid::symtab-sh-info.s
Script: -- : 'RUN: at line 4'; c:\ws\w16c2-1\llvm-project\premerge-checks\build\bin\yaml2obj.exe --docnum=1 C:\ws\w16c2-1\llvm-project\premerge-checks\lld\test\ELF\invalid\symtab-sh-info.s -o C:\ws\w16c2-1\llvm-project\premerge-checks\build\tools\lld\test\ELF\invalid\Output\symtab-sh-info.s.tmp.o

Event Timeline

mbrkusanin created this revision.Oct 27 2020, 4:41 AM
mbrkusanin requested review of this revision.Oct 27 2020, 4:41 AM

This patch is basically a global-isel version of combineShiftOfShiftedLogic() from DAGCombiner.


This can be simplified to a single instruction:
s_lshl_b32 s0, s0, 4
but that will be a different combine.

Same goes for a few more cases below.

  • clang-format
foad added inline comments.Oct 28 2020, 2:10 AM

I know you copied this comment from SelectionDAG but I think it really improves latency, not throughput, because the new shifts are independent.

Also you don't mention the other reason for doing this, which is that one of the new shifts might constant-fold away.


Also handle G_USHLSAT and G_SSHLSAT?


I don't see why this check is necessary. All we care about is the shift amount, as a uint64_t.


First, it would be simpler to check this by line 1690 where you're already computing the sum. Second, I would expect the > to be >=. Third, you're comparing against BitWidth which is the width of the shift amount, not the value being shifted.


Don't create this. CombinerHelper already has a MIRBuilder.


ShlType is wrong here and below. You want the type of the result, not the type of the shift amount.


It's probably not worth doing this. I think you can rely on dead code elimination.

mbrkusanin marked 5 inline comments as done.
  • Supported G_SSHLSAT and G_USHLSAT and addressed other comments.
  • Added .mir test for G_SSHLSAT and G_USHLSAT.
foad accepted this revision.Nov 2 2020, 7:02 AM
foad added inline comments.

Comment still needs updating.

This revision is now accepted and ready to land.Nov 2 2020, 7:02 AM
mbrkusanin updated this revision to Diff 302526.Nov 3 2020, 2:51 AM
  • Updated comments.
arsenm accepted this revision.Nov 3 2020, 7:42 AM