Page MenuHomePhabricator

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

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

Details

Summary

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

TimeTest
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.

llvm/test/CodeGen/AMDGPU/GlobalISel/combine-shift-of-shifted-logic.ll
7–8

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
llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
248–249

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.

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1609

Also handle G_USHLSAT and G_SSHLSAT?

1653–1656

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

1664–1666

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.

1700

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

1707

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

1717–1719

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.
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
1609

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