[AMDGPU] Add a new Clamp Pattern to the GlobalISel Path.

[tsymalla]

Summary

This patch should find a pattern in GlobalISel / AMDGPUPostLegalizerCombiner which can appear when trying to clamp 64-bit values to short values without truncation, for example by using the 64-bit / 16-bit integer extensions in shaders. This pattern can be reduced to two VALU instructions:

v_cvt_pk_i16_i32_e64 v0, v0, v1 ; where v[0:1] is a 64-bit number
v_med3_i32 v0, LowBoundary, v0, HighBoundary ; where LowBoundary >= SHORT_MII, HighBoundary <= SHORT_MAX

I am open for suggestions and reviews as this is my first change on the LLVM compiler.

[Flakebi]

Looks good to me, I left some nits inline.
Someone who is more familiar with GlobalISel should review this.

llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp

1–2

Unintended change?

llvm/test/CodeGen/AMDGPU/GlobalISel/clampi64toi16.ll

3 ↗

(On Diff #313345)

GFX9 is also a GCN architecture, so calling this GFX10 might be a clearer choice.

[Petar.Avramovic]

llvm/test/CodeGen/AMDGPU/GlobalISel/clampi64toi16.ll

11 ↗

(On Diff #313345)

This looks incorrect since some high bits are discarded (are you missing some known bits analysis?).
Think of positive signed input value that has 1 somewhere in high 16 bits, e.g. 0x4000000000000000
'icmp select' sequence gives 32767 which is then truncated to i16 (no op in this case).
But cvt_pk takes low 16 bits from 0x40000000 and 0x00000000 and packs them into 32 bit value 0.
v_med3 with [[A]]=0 gives 0.
Does sdag have a similar combine?

16–17 ↗

(On Diff #313345)

For awareness: generic llvm-ir max and min intrinsics @llvm.umax. and @llvm.umin. were added recently.
It is expected to get llvm-ir combine that will transform cmp+select into intrinsic at some point and then there would be no need to match cmp+select in combines. You should test with min and max intrinsics for now. However since this is i64 I guess it is fine to use cmp + select since intrinsic min and max have to be lowered for s64.

[arsenm]

llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h

475

This is doing too much, this should just match the G_UMIN/G_UMAX/G_SMIN/G_SMAX opcodes. This shouldn't be responsible for matching the compare+select pattern

llvm/test/CodeGen/AMDGPU/GlobalISel/clampi64toi16.ll

1 ↗

(On Diff #313345)

I would prefer if the test name had a more consistent name. How about combine-int-clamp.ll?

18–21 ↗

(On Diff #313345)

I dislike anonymous values in tests, can you add names here

[nhaehnle]

llvm/test/CodeGen/AMDGPU/GlobalISel/clampi64toi16.ll

11 ↗

(On Diff #313345)

cvt_pk performs a clamp before packing into 16 bits. It's not very well documented.

[tsymalla]

llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp

1–2

I think this happened because of using clang-format.

llvm/test/CodeGen/AMDGPU/GlobalISel/clampi64toi16.ll

11 ↗

(On Diff #313345)

The whole concept behind this, as Nicolai said, is, to have the two 32 bit-parts of the 64-bit number being clamped automatically against short boundaries by using v_cvt_pk_i16_i32. So the lower and higher 16 bits of the resulting 32 bit number should be clamped independently. If this fails for some case (very large numbers), the v_med3 instruction should make sure that the value actually gets clamped.

18–21 ↗

(On Diff #313345)

Hi, what do you mean by anonymous values in this context?

[tsymalla]

Mostly renamings.

[tsymalla]

Refactored the pattern matching algorithm. I removed the construct from MIPatternMatch and implemented it directly in the PostLegalizerCombiner.

[tsymalla]

llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h

475

I removed this construct. There area helpers already which could be extended for matching the G_*MAX/MIN opcodes. I tried to implement this straight forward directly in the matcher.

[arsenm]

llvm/test/CodeGen/AMDGPU/GlobalISel/clampi64toi16.ll

18–21 ↗

(On Diff #313345)

The numbered %0, %1. They should have fixed string names instead. opt -S -instnamer will do this for you or you can manually %name them

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp

2

This has too many -s then

285

This should still be trying to match against the min/max opcodes, not looking for compare and selects

[tsymalla]

Renamed the identifiers in the lit test.

llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp

2

I reverted this change.

285

So, if I understand this correctly:

• The select/compare pattern I'm trying to match here will be matched to G_MIN/G_MAX
• This seems to be not implemented yet or not working for my case (as far as I can see that)

I'm not sure what should be achieved here. Should there either be a "future-proof" implementation for matching min and max which is not working yet, an additional case for matching min and max which falls back to my current implementation or a combination of a) extension of the pattern matching that combines select and icmp to max / min and b) an implementation which only matches max / min and combines it accordingly?
Would you mind explaining this to me a little bit more?

llvm/test/CodeGen/AMDGPU/GlobalISel/clampi64toi16.ll

18–21 ↗

(On Diff #313345)

Thanks. I manually named them.

[foad]

llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp

15–16

You can use the git-clang-format script to avoid reformatting the whole file. If you put it on your path then you can invoke it as e.g. git clang-format @^ to only reformat the parts of the file that were changed by your most recent commit.

311–319

Use // even for block comments please.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp

285

The compare+select pattern you are matching here should be matched separately into the min/max opcodes as a better, more canonical form. This is not implemented in GlobalISel yet.

Matching min/max should be implemented in GlobalISel, however I don't think this is a priority. Theoretically the IR optimizers should be matching these in the vast majority of cases with the new min/max intrinsics. A GlobalISel combine would theoretically be useful if the compare/select pattern were to be emitted by independent legalizations, but I'm currently not aware of any implemented case where this could happen

[tsymalla]

llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp

285

So, what would be the right approach to handle this in your opinion? As far as I understand it doesn't make sense to have an implementation for this specific case where a generic implementation would be more suitable. Would you say it makes more sense to wait for the max / min matching in GlobalISel and implement my combine around it?

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp

285

You don't need to wait. You can just do it with the min/max opcodes now. You can either write MIR tests or change the IR tests to use the intrinsics already

[tsymalla]

llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp

311–319

Thanks, Jay. I will change this.

[tsymalla]

This moves the implementation to the PreLegalizer and is based on LLVM max/min intrinsics.

[arsenm]

I would expect this in the post legalizer combiner, and going directly from i64 is a bit weird. What does the post-legalized MIR look like for this?

llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h

455

Remove leftover whitespace change

llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp

191–192

Unrelated formatting changes

248

Extra whitespace change

llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp

62 ↗

(On Diff #315783)

Capitalize

74 ↗

(On Diff #315783)

Shouldn't construct single use MachineIRBuilder, there should be one in the combiner state

104 ↗

(On Diff #315783)

Capitalize

119 ↗

(On Diff #315783)

This is just noise, remove it

121 ↗

(On Diff #315783)

Don't construct fresh builder

129 ↗

(On Diff #315783)

You can just do Unmerge.getReg(0)

134 ↗

(On Diff #315783)

It would be better to use a generic operation pre-regbank select rather than forcing register classes here

148–149 ↗

(On Diff #315783)

Bad shadowing name

151–153 ↗

(On Diff #315783)

You can combine this as auto MinBoundaryDst = B.buildConstant(S32, min)

[tsymalla]

Previously this was implemented in the PostLegalizer. The SMAX / SMIN MIR gets lowered to a pattern of G_SELECT / G_ICMP, so the state after PostLegalizing is equivalent to what I've implemented in the tests before matching this to the min/max instructions. Disabling the optimization in the implementation here yields a result similar to the following:

%62:_(s1) = G_ICMP intpred(slt), %53:_(s32), %25:_
%63:_(s64) = G_CONSTANT i64 0
%28:_(s64) = G_SELECT %62:_(s1), %63:_, %61:_
%29:_(s64) = G_CONSTANT i64 32767
%37:_(s1) = G_ICMP intpred(slt), %28:_(s64), %29:_
%30:_(s64) = G_SELECT %37:_(s1), %28:_, %29:_
%31:_(s64) = G_CONSTANT i64 -32768
%36:_(s1) = G_ICMP intpred(sgt), %30:_(s64), %31:_
%32:_(s64) = G_SELECT %36:_(s1), %30:_, %31:_
%34:_(s32) = G_TRUNC %32:_(s64)

So this is why I moved the implementation to the PreLegalize step where the G_SMIN / G_SMAX IR still exists:

%30:_(s64) = G_SMIN %28:_, %29:_
%32:_(s64) = G_SMAX %30:_, %31:_
%33:_(s16) = G_TRUNC %32:_(s64)

I don't exactly understand why the min / max instructions get lowered to this pattern so for the moment, the only way to get this producing the code I wish seems to match the pattern in the PreLegalize step. By the way, the goal here is to check explicitly if an long should get clamped to short boundaries (or inbetween). Instead of producing some comparisons, this should yield these two instructions as I've figured out with Nicolai.

llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp

134 ↗

(On Diff #315783)

How would I go for this?

[tsymalla]

Various changes according to the code review. I don't know why the formatting changes in the PostLegalizer keep showing up as they are already reverted to HEAD...

[arsenm]

Do we do the equivalent combine in the DAG path? I don't recognize this one

llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp

71 ↗

(On Diff #316057)

Probably shouldn't print here especially when failure is likely. Ideally we would get a message from the generated combiner based on the defined combine name

132 ↗

(On Diff #316057)

I'd rather just repeat the VGPR_32RegClass (also shuoldn't use macro-like naming convention)

134 ↗

(On Diff #315783)

You can follow the example of the other G_AMDGPU_* pseudos (e.g. G_AMDGPU_CVT_UBYTE0). I'm a bit ambivalent on whether we should do this in this case, as there are still some unanswered questions about how regbankselect should work. However, so far we've been using pseudos like this

llvm/test/CodeGen/AMDGPU/GlobalISel/combine-short-clamp.ll

124

Fix this whitespace error

[Petar.Avramovic]

llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp

137–141 ↗

(On Diff #316057)

You can replace this block with B.buildInstr(CvtOpcode, {CvtDst}, {Hi32, Lo32}, MI.getFlags());

201 ↗

(On Diff #316057)

Why do you need LegalizerInfo? CombinerHelper::isLegalOrBeforeLegalizer relies on LI being nullptr before legalizer.

[tsymalla]

No, I doubt there is support for this in the other path.

llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp

71 ↗

(On Diff #316057)

I removed this message. There aren't any other debug outputs in the other Legalize combines, too.

137–141 ↗

(On Diff #316057)

Thanks. I removed this for the other buildInstr, too.

201 ↗

(On Diff #316057)

Thanks. This is not needed here, so I removed it.

134 ↗

(On Diff #315783)

Thanks. With some help I was able to get this working and learned something new. This should now use the new pseudo and select it into the corresponding machine instruction.

[tsymalla]

Implemented a GMIR instruction for the V_CVT opcode and did some changes to the implementation according to the code reviews.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp

149 ↗

(On Diff #316380)

A pseudo here would also apply for the same reasons

[tsymalla]

llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp

149 ↗

(On Diff #316380)

I will implement this as soon as I'll get access back to my machine. Is there a reason why in some cases pseudos exist, in some cases not?

[tsymalla]

This adds an additional G_MED3_S32 GIR opcode and uses it in the PreLegalizerCombiner.

[tsymalla]

Added the new MED3 pseudo.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp

146 ↗

(On Diff #317348)

Don't need the register classes. You can also hide the createRegister calls with the builder as well

149 ↗

(On Diff #316380)

If we need to change how the operation is selected based on the register banks, it definitely should use a regbankselectable pseudo with generic register operands. If there isn't a meaningful handling change depending on the register banks, there's less reason to use a pseudo.

[tsymalla]

llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp

146 ↗

(On Diff #317348)

Thanks. I reworked this aspect of the implementation. Should be a bit more clean now.

149 ↗

(On Diff #316380)

Thanks. :)

[tsymalla]

Removes the generic createVirtualRegister invocations and the manual setting of the VGPR32 register class.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp

142–143 ↗

(On Diff #318987)

You can truncate directly into the destination register, you don't need this copy

llvm/lib/Target/AMDGPU/SIInstructions.td

2596 ↗

(On Diff #318987)

No _S32, this is also available for s16 on some targets

[tsymalla]

Renamed the G_MED3 opcode and removed a superfluous copy.

[tsymalla]

Hi, it seems, I am not permitted to commit this patch. Could somebody please do the commit for me? Thanks!

[arsenm]

In D93708#2524681, @tsymalla wrote:

Hi, it seems, I am not permitted to commit this patch. Could somebody please do the commit for me? Thanks!

You have to request commit access:
https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

[tsymalla]

Requested access. Thanks.

[arsenm]

I've tried applying this but getting some build errors. You also have some remaining trailing whitespace and extra whitespace changes

[tsymalla]

In D93708#2525719, @arsenm wrote:

I've tried applying this but getting some build errors. You also have some remaining trailing whitespace and extra whitespace changes

I will revert those whitespace changes, thanks.
Where did you apply this? Locally? For me, this worked with a recent LLVM version.

[tsymalla]

I investigated the problem and it seems, there were some includes missing in PostLegalizerCombiner as there were changes in terms of how the instructions are made available to the TU.
I will fix this and upload a new patch.

[tsymalla]

Reverted whitespace changes, added missing includes.

[eugenis]

Hi,

This change, or one of the related change has broken the msan bot.
http://lab.llvm.org:8011/#/builders/5/builds/4134

==91167==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x35d38b6 in AMDGPUPreLegalizerCombinerHelper::matchClampI64ToI16(llvm::MachineInstr&, llvm::MachineRegisterInfo&, llvm::MachineFunction&, AMDGPUPreLegalizerCombinerHelper::ClampI64ToI16MatchInfo&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp:92:17
    #1 0x35e7a91 in operator() /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/lib/Target/AMDGPU/AMDGPUGenPreLegalizeGICombiner.inc:1227:38
    #2 0x35e7a91 in (anonymous namespace)::AMDGPUGenPreLegalizerCombinerHelper::tryCombineAll(llvm::GISelChangeObserver&, llvm::MachineInstr&, llvm::MachineIRBuilder&, llvm::CombinerHelper&) const /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/lib/Target/AMDGPU/AMDGPUGenPreLegalizeGICombiner.inc:1226:14
    #3 0x35d6aed in (anonymous namespace)::AMDGPUPreLegalizerCombinerInfo::combine(llvm::GISelChangeObserver&, llvm::MachineInstr&, llvm::MachineIRBuilder&) const /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp:197:17
    #4 0xae8a3c4 in llvm::Combiner::combineMachineInstrs(llvm::MachineFunction&, llvm::GISelCSEInfo*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/CodeGen/GlobalISel/Combiner.cpp:150:24
    #5 0x35d629f in (anonymous namespace)::AMDGPUPreLegalizerCombiner::runOnMachineFunction(llvm::MachineFunction&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp:268:12
    #6 0x71117ca in llvm::MachineFunctionPass::runOnFunction(llvm::Function&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/CodeGen/MachineFunctionPass.cpp:72:13
    #7 0x7f07777 in llvm::FPPassManager::runOnFunction(llvm::Function&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1435:27
    #8 0x63360b8 in RunPassOnSCC /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Analysis/CallGraphSCCPass.cpp:178:25
    #9 0x63360b8 in RunAllPassesOnSCC /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Analysis/CallGraphSCCPass.cpp:476:9
    #10 0x63360b8 in (anonymous namespace)::CGPassManager::runOnModule(llvm::Module&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Analysis/CallGraphSCCPass.cpp:541:18
    #11 0x7f08ee4 in runOnModule /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1550:27
    #12 0x7f08ee4 in llvm::legacy::PassManagerImpl::run(llvm::Module&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:541:44
    #13 0x27ee02a in compileModule(char**, llvm::LLVMContext&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/tools/llc/llc.cpp:684:8
    #14 0x27e7c41 in main /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/tools/llc/llc.cpp:385:22
    #15 0x7f1bd530d09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    #16 0x2767739 in _start (/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/llc+0x2767739)

Also, there is something very wrong with the diff here. It just shows a bunch of merge conflict markers. Please make sure that your commits include the relevant headers like "Differential Revision", etc - it's very hard to track things otherwise.

[arsenm]

In D93708#2537793, @eugenis wrote:

Hi,

This change, or one of the related change has broken the msan bot.
http://lab.llvm.org:8011/#/builders/5/builds/4134

==91167==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x35d38b6 in AMDGPUPreLegalizerCombinerHelper::matchClampI64ToI16(llvm::MachineInstr&, llvm::MachineRegisterInfo&, llvm::MachineFunction&, AMDGPUPreLegalizerCombinerHelper::ClampI64ToI16MatchInfo&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp:92:17
    #1 0x35e7a91 in operator() /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/lib/Target/AMDGPU/AMDGPUGenPreLegalizeGICombiner.inc:1227:38
    #2 0x35e7a91 in (anonymous namespace)::AMDGPUGenPreLegalizerCombinerHelper::tryCombineAll(llvm::GISelChangeObserver&, llvm::MachineInstr&, llvm::MachineIRBuilder&, llvm::CombinerHelper&) const /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/lib/Target/AMDGPU/AMDGPUGenPreLegalizeGICombiner.inc:1226:14
    #3 0x35d6aed in (anonymous namespace)::AMDGPUPreLegalizerCombinerInfo::combine(llvm::GISelChangeObserver&, llvm::MachineInstr&, llvm::MachineIRBuilder&) const /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp:197:17
    #4 0xae8a3c4 in llvm::Combiner::combineMachineInstrs(llvm::MachineFunction&, llvm::GISelCSEInfo*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/CodeGen/GlobalISel/Combiner.cpp:150:24
    #5 0x35d629f in (anonymous namespace)::AMDGPUPreLegalizerCombiner::runOnMachineFunction(llvm::MachineFunction&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp:268:12
    #6 0x71117ca in llvm::MachineFunctionPass::runOnFunction(llvm::Function&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/CodeGen/MachineFunctionPass.cpp:72:13
    #7 0x7f07777 in llvm::FPPassManager::runOnFunction(llvm::Function&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1435:27
    #8 0x63360b8 in RunPassOnSCC /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Analysis/CallGraphSCCPass.cpp:178:25
    #9 0x63360b8 in RunAllPassesOnSCC /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Analysis/CallGraphSCCPass.cpp:476:9
    #10 0x63360b8 in (anonymous namespace)::CGPassManager::runOnModule(llvm::Module&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Analysis/CallGraphSCCPass.cpp:541:18
    #11 0x7f08ee4 in runOnModule /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:1550:27
    #12 0x7f08ee4 in llvm::legacy::PassManagerImpl::run(llvm::Module&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:541:44
    #13 0x27ee02a in compileModule(char**, llvm::LLVMContext&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/tools/llc/llc.cpp:684:8
    #14 0x27e7c41 in main /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/tools/llc/llc.cpp:385:22
    #15 0x7f1bd530d09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    #16 0x2767739 in _start (/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/llc+0x2767739)

Also, there is something very wrong with the diff here. It just shows a bunch of merge conflict markers. Please make sure that your commits include the relevant headers like "Differential Revision", etc - it's very hard to track things otherwise.

I think that's what D95878 is for

[sebastian-ne]

It may take some time until @tsymalla can look at the fix in D95878 again, so I reverted this for the time being.

llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h

                          TargetOpcode::G_INSERT_VECTOR_ELT>(Src0, Src1, Src2);
 }
 
+template <typename Src0Ty, typename Src1Ty, typename Src2Ty>
+inline TernaryOp_match<Src0Ty, Src1Ty, Src2Ty, TargetOpcode::G_SELECT>
+m_GISelect(const Src0Ty &Src0, const Src1Ty &Src1, const Src2Ty &Src2) {
+  return TernaryOp_match<Src0Ty, Src1Ty, Src2Ty, TargetOpcode::G_SELECT>(
+      Src0, Src1, Src2);
+}
+
 /// Matches a register negated by a G_SUB.
 /// G_SUB 0, %negated_reg
 template <typename SrcTy>

   return m_GXor(Src, m_AllOnesInt());
 }
 
-} // namespace GMIPatternMatch
+
+} // namespace MIPatternMatch
 } // namespace llvm
 
 #endif

llvm/lib/Target/AMDGPU/AMDGPUCombine.td

          [{ return PostLegalizerHelper.matchCvtF32UByteN(*${cvt_f32_ubyteN}, ${matchinfo}); }]),
   (apply [{ PostLegalizerHelper.applyCvtF32UByteN(*${cvt_f32_ubyteN}, ${matchinfo}); }])>;
 
+def clamp_i64_to_i16_matchdata : GIDefMatchData<"AMDGPUPostLegalizerCombinerHelper::ClampI64ToI16MatchInfo">;
+
+def clamp_i64_to_i16 : GICombineRule<
+  (defs root:$clamp_i64_to_i16, clamp_i64_to_i16_matchdata:$matchinfo),
+  (match (wip_match_opcode G_TRUNC):$clamp_i64_to_i16,
+      [{ return PostLegalizerHelper.matchClampI64ToI16(*${clamp_i64_to_i16}, MRI, *MF, ${matchinfo}); }]),
+  (apply [{ PostLegalizerHelper.applyClampI64ToI16(*${clamp_i64_to_i16}, ${matchinfo}); }])>;
+
 // Combines which should only apply on SI/VI
 def gfx6gfx7_combines : GICombineGroup<[fcmp_select_to_fmin_fmax_legacy]>;
 

 def AMDGPUPostLegalizerCombinerHelper: GICombinerHelper<
   "AMDGPUGenPostLegalizerCombinerHelper",
   [all_combines, gfx6gfx7_combines,
-   uchar_to_float, cvt_f32_ubyteN]> {
+   uchar_to_float, cvt_f32_ubyteN, clamp_i64_to_i16]> {
   let DisableRuleOption = "amdgpupostlegalizercombiner-disable-rule";
   let StateClass = "AMDGPUPostLegalizerCombinerHelperState";
   let AdditionalArguments = [];

llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp

 //
 //===----------------------------------------------------------------------===//
 
-#include "AMDGPUTargetMachine.h"
 #include "AMDGPULegalizerInfo.h"
+#include "AMDGPUTargetMachine.h"
+#include "MCTargetDesc/AMDGPUMCTargetDesc.h"
 #include "llvm/CodeGen/GlobalISel/Combiner.h"
 #include "llvm/CodeGen/GlobalISel/CombinerHelper.h"
 #include "llvm/CodeGen/GlobalISel/CombinerInfo.h"

 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/TargetPassConfig.h"
 #include "llvm/Support/Debug.h"
-#include "MCTargetDesc/AMDGPUMCTargetDesc.h"
 
 #define DEBUG_TYPE "amdgpu-postlegalizer-combiner"
 

   bool matchCvtF32UByteN(MachineInstr &MI, CvtF32UByteMatchInfo &MatchInfo);
   void applyCvtF32UByteN(MachineInstr &MI,
                          const CvtF32UByteMatchInfo &MatchInfo);
+
+  struct ClampI64ToI16MatchInfo {
+    int64_t Cmp1;
+    int64_t Cmp2;
+    Register Origin;
+  };
+
+  bool matchClampI64ToI16(MachineInstr &MI, MachineRegisterInfo &MRI,
+                          MachineFunction &MF,
+                          ClampI64ToI16MatchInfo &MatchInfo);
+
+  void applyClampI64ToI16(MachineInstr &MI,
+                          const ClampI64ToI16MatchInfo &MatchInfo);
 };
 
 bool AMDGPUPostLegalizerCombinerHelper::matchFMinFMaxLegacy(

     SrcReg = B.buildAnyExtOrTrunc(S32, SrcReg).getReg(0);
 
   if (Ty == S32) {
-    B.buildInstr(AMDGPU::G_AMDGPU_CVT_F32_UBYTE0, {DstReg},
-                   {SrcReg}, MI.getFlags());
+    B.buildInstr(AMDGPU::G_AMDGPU_CVT_F32_UBYTE0, {DstReg}, {SrcReg},
+                 MI.getFlags());
   } else {
-    auto Cvt0 = B.buildInstr(AMDGPU::G_AMDGPU_CVT_F32_UBYTE0, {S32},
-                             {SrcReg}, MI.getFlags());
+    auto Cvt0 = B.buildInstr(AMDGPU::G_AMDGPU_CVT_F32_UBYTE0, {S32}, {SrcReg},
+                             MI.getFlags());
     B.buildFPTrunc(DstReg, Cvt0, MI.getFlags());
   }
 

   MI.eraseFromParent();
 }
 
+bool AMDGPUPostLegalizerCombinerHelper::matchClampI64ToI16(
+    MachineInstr &MI, MachineRegisterInfo &MRI, MachineFunction &MF,
+    ClampI64ToI16MatchInfo &MatchInfo) {
+  assert(MI.getOpcode() == TargetOpcode::G_TRUNC && "Invalid instruction!");
+  const LLT SrcType = MRI.getType(MI.getOperand(1).getReg());
+
+  // we want to check if a 64-bit number gets clamped to 16-bit boundaries (or
+  // below).
+  if (SrcType != LLT::scalar(64))
+    return false;
+
+  MachineIRBuilder B(MI);
+
+  LLVM_DEBUG(dbgs() << "Matching Clamp i64 to i16");
+
+  CmpInst::Predicate Predicate1;
+  Register Base;
+
+  if (!mi_match(MI.getOperand(1).getReg(), MRI, m_GISelect(m_GICmp(m_Pred(Predicate1), m_Reg(), m_Reg()), m_Reg(Base), m_ICst(MatchInfo.Cmp1))))
+    return false;
+
+  CmpInst::Predicate Predicate2;
+
+  if (!mi_match(Base, MRI, m_GISelect(m_GICmp(m_Pred(Predicate2), m_Reg(), m_Reg()), m_Reg(MatchInfo.Origin), m_ICst(MatchInfo.Cmp2))))
+    return false;
+    
+  if ((Predicate1 == CmpInst::ICMP_SLT &&
+      Predicate2 == CmpInst::ICMP_SGT) ||
+      (Predicate1 == CmpInst::ICMP_SGT &&
+      Predicate2 == CmpInst::ICMP_SLT)) {
+    const auto Cmp1 = MatchInfo.Cmp1;
+    const auto Cmp2 = MatchInfo.Cmp2;
+    const auto Diff = std::abs(Cmp2 - Cmp1);
+
+    // we don't need to clamp here.
+    if (Diff == 0 || Diff == 1) {
+      return false;
+    }
+
+    const int64_t Min = std::numeric_limits<int16_t>::min();
+    const int64_t Max = std::numeric_limits<int16_t>::max();
+
+    // are we really trying to clamp against the relevant boundaries?
+    return ((Cmp2 >= Cmp1 && Cmp1 >= Min && Cmp2 <= Max) ||
+            (Cmp1 >= Cmp2 && Cmp1 <= Max && Cmp2 >= Min));
+  }
+
+  return false;
+}
+
+/**
+ * We want to find a combination of instructions that
+ * gets generated when an i64 gets clamped to i16.
+ * The corresponding pattern is:
+ * G_SELECT MIN/MAX, G_ICMP, G_SELECT MIN/MAX, G_ICMP, G_TRUNC.
+ * This can be efficiently written as following:
+ * v_cvt_pk_i16_i32 v0, v0, v1
+ * v_med3_i32 v0, Clamp_Min, v0, Clamp_Max
+ */
+void AMDGPUPostLegalizerCombinerHelper::applyClampI64ToI16(
+    MachineInstr &MI, const ClampI64ToI16MatchInfo &MatchInfo) {
+  LLVM_DEBUG(dbgs() << "Combining MI");
+
+  MachineIRBuilder B(MI);
+  MachineRegisterInfo &MRI = MI.getParent()->getParent()->getRegInfo();
+
+  Register Src = MatchInfo.Origin;
+  assert(MRI.getType(Src) == LLT::scalar(64));
+  const LLT S32 = LLT::scalar(32);
+
+  auto Unmerge = B.buildUnmerge(S32, Src);
+  Register Hi32 = Unmerge->getOperand(0).getReg();
+  Register Lo32 = Unmerge->getOperand(1).getReg();
+  MRI.setRegClass(Hi32, &AMDGPU::VGPR_32RegClass);
+  MRI.setRegClass(Lo32, &AMDGPU::VGPR_32RegClass);
+
+  constexpr unsigned int CvtOpcode = AMDGPU::V_CVT_PK_I16_I32_e64;
+  assert(MI.getOpcode() != CvtOpcode);
+
+  const auto REG_CLASS = &AMDGPU::VGPR_32RegClass;
+
+  Register CvtDst = MRI.createVirtualRegister(REG_CLASS);
+  MRI.setType(CvtDst, S32);
+
+  auto CvtPk = B.buildInstr(CvtOpcode);
+  CvtPk.addDef(CvtDst);
+  CvtPk.addReg(Hi32);
+  CvtPk.addReg(Lo32);
+  CvtPk.setMIFlags(MI.getFlags());
+
+  auto min = std::min(MatchInfo.Cmp1, MatchInfo.Cmp2);
+  auto max = std::max(MatchInfo.Cmp1, MatchInfo.Cmp2);
+
+  Register MinBoundaryDst = MRI.createVirtualRegister(REG_CLASS);
+  MRI.setType(MinBoundaryDst, S32);
+  B.buildConstant(MinBoundaryDst, min);
+
+  Register MaxBoundaryDst = MRI.createVirtualRegister(REG_CLASS);
+  MRI.setType(MaxBoundaryDst, S32);
+  B.buildConstant(MaxBoundaryDst, max);
+
+  Register MedDst = MRI.createVirtualRegister(REG_CLASS);
+  MRI.setType(MedDst, S32);
+
+  auto Med = B.buildInstr(AMDGPU::V_MED3_I32);
+  Med.addDef(MedDst);
+  Med.addReg(MinBoundaryDst);
+  Med.addReg(CvtDst);
+  Med.addReg(MaxBoundaryDst);
+  Med.setMIFlags(MI.getFlags());
+
+  B.buildCopy(MI.getOperand(0).getReg(), MedDst);
+
+  MI.eraseFromParent();
+}
+
 class AMDGPUPostLegalizerCombinerHelperState {
 protected:
   CombinerHelper &Helper;

   bool runOnMachineFunction(MachineFunction &MF) override;
 
   void getAnalysisUsage(AnalysisUsage &AU) const override;
+
 private:
   bool IsOptNone;
 };

 }
 
 AMDGPUPostLegalizerCombiner::AMDGPUPostLegalizerCombiner(bool IsOptNone)
     : MachineFunctionPass(ID), IsOptNone(IsOptNone) {
   initializeAMDGPUPostLegalizerCombinerPass(*PassRegistry::getPassRegistry());
 }
 

       MF.getTarget().getOptLevel() != CodeGenOpt::None && !skipFunction(F);
 
   const GCNSubtarget &ST = MF.getSubtarget<GCNSubtarget>();
-  const AMDGPULegalizerInfo *LI
-    = static_cast<const AMDGPULegalizerInfo *>(ST.getLegalizerInfo());
+  const AMDGPULegalizerInfo *LI =
+      static_cast<const AMDGPULegalizerInfo *>(ST.getLegalizerInfo());
 
   GISelKnownBits *KB = &getAnalysis<GISelKnownBitsAnalysis>().get(MF);
   MachineDominatorTree *MDT =

 
 char AMDGPUPostLegalizerCombiner::ID = 0;
 INITIALIZE_PASS_BEGIN(AMDGPUPostLegalizerCombiner, DEBUG_TYPE,
-                      "Combine AMDGPU machine instrs after legalization",
-                      false, false)
+                      "Combine AMDGPU machine instrs after legalization", false,
+                      false)
 INITIALIZE_PASS_DEPENDENCY(TargetPassConfig)
 INITIALIZE_PASS_DEPENDENCY(GISelKnownBitsAnalysis)
 INITIALIZE_PASS_END(AMDGPUPostLegalizerCombiner, DEBUG_TYPE,

llvm/test/CodeGen/AMDGPU/GlobalISel/combine-short-clamp.ll

+; RUN: llc -global-isel -mcpu=tahiti -mtriple=amdgcn-amd-amdhsa -verify-machineinstrs < %s | FileCheck --check-prefixes=GFX678,GFX6789 %s
+; RUN: llc -global-isel -mcpu=gfx900 -mtriple=amdgcn-amd-amdhsa -verify-machineinstrs < %s | FileCheck --check-prefixes=GFX9,GFX6789 %s
+; RUN: llc -global-isel -mcpu=gfx1010 -march=amdgcn -verify-machineinstrs < %s | FileCheck --check-prefix=GFX10 %s
+
+; GFX10-LABEL: {{^}}v_clamp_i64_i16
+; GFX678: v_cvt_pk_i16_i32_e32 [[A:v[0-9]+]], [[A]], [[B:v[0-9]+]]
+; GFX9: v_cvt_pk_i16_i32 [[A:v[0-9]+]], [[A]], [[B:v[0-9]+]]
+; GFX6789: v_mov_b32_e32 [[B]], 0xffff8000
+; GFX6789: v_mov_b32_e32 [[C:v[0-9]+]], 0x7fff
+; GFX6789: v_med3_i32 [[A]], [[B]], [[A]], [[C]]
+; GFX10: v_cvt_pk_i16_i32_e64 [[A:v[0-9]+]], [[A]], [[B:v[0-9]+]]
+; GFX10: v_mov_b32_e32 [[C:v[0-9]+]], 0x7fff
+; GFX10: v_med3_i32 [[A]], 0xffff8000, [[A]], [[C]]
+define i16 @v_clamp_i64_i16(i64 %in) #0 {
+entry:
+  %greater = icmp sgt i64 %in, -32768
+  %i1 = select i1 %greater, i64 %in, i64 -32768
+  %lower = icmp slt i64 %i1, 32767
+  %intermed = select i1 %lower, i64 %i1, i64 32767
+  %result = trunc i64 %intermed to i16
+  ret i16 %result
+}
+
+; GFX10-LABEL: {{^}}v_clamp_i64_i16_reverse
+; GFX678: v_cvt_pk_i16_i32_e32 [[A:v[0-9]+]], [[A]], [[B:v[0-9]+]]
+; GFX9: v_cvt_pk_i16_i32 [[A:v[0-9]+]], [[A]], [[B:v[0-9]+]]
+; GFX6789: v_mov_b32_e32 [[B]], 0xffff8000
+; GFX6789: v_mov_b32_e32 [[C:v[0-9]+]], 0x7fff
+; GFX6789: v_med3_i32 [[A]], [[B]], [[A]], [[C]]
+; GFX10: v_cvt_pk_i16_i32_e64 [[A:v[0-9]+]], [[A]], [[B:v[0-9]+]]
+; GFX10: v_mov_b32_e32 [[C:v[0-9]+]], 0x7fff
+; GFX10: v_med3_i32 [[A]], 0xffff8000, [[A]], [[C]]
+define i16 @v_clamp_i64_i16_reverse(i64 %in) #0 {
+entry:
+  %lower = icmp slt i64 %in, 32767
+  %i1 = select i1 %lower, i64 %in, i64 32767
+  %greater = icmp sgt i64 %i1, -32768
+  %intermed = select i1 %greater, i64 %i1, i64 -32768
+  %result = trunc i64 %intermed to i16
+  ret i16 %result
+}
+
+; GFX10-LABEL: {{^}}v_clamp_i64_i16_wrong_lower
+; GFX6789: v_mov_b32_e32 [[B:v[0-9]+]], 0x8001
+; GFX6789: v_cndmask_b32_e32 [[A:v[0-9]+]], [[B]], [[A]], vcc
+; GFX6789: v_cndmask_b32_e32 [[C:v[0-9]+]], 0, [[C]], vcc
+
+; GFX10: v_cndmask_b32_e32 [[A:v[0-9]+]], 0x8001, [[A]], vcc_lo
+; GFX10: v_cndmask_b32_e32 [[B:v[0-9]+]], 0, [[B]], vcc_lo
+define i16 @v_clamp_i64_i16_wrong_lower(i64 %in) #0 {
+entry:
+  %lower = icmp slt i64 %in, 32769
+  %i1 = select i1 %lower, i64 %in, i64 32769
+  %greater = icmp sgt i64 %i1, -32768
+  %intermed = select i1 %greater, i64 %i1, i64 -32768
+  %result = trunc i64 %intermed to i16
+  ret i16 %result
+}
+
+; GFX10-LABEL: {{^}}v_clamp_i64_i16_wrong_lower_and_higher
+; GFX6789: v_mov_b32_e32 [[B:v[0-9]+]], 0x8000
+; GFX6789: v_cndmask_b32_e32 [[A:v[0-9]+]], [[B]], [[A]], vcc
+; GFX10: v_cndmask_b32_e32 [[A:v[0-9]+]], 0x8000, [[A]], vcc_lo
+define i16 @v_clamp_i64_i16_wrong_lower_and_higher(i64 %in) #0 {
+entry:
+  %greater = icmp sgt i64 %in, -32769
+  %i1 = select i1 %greater, i64 %in, i64 -32769
+  %lower = icmp slt i64 %i1, 32768
+  %intermed = select i1 %lower, i64 %i1, i64 32768
+  %result = trunc i64 %intermed to i16
+  ret i16 %result
+}
+
+; GFX10-LABEL: {{^}}v_clamp_i64_i16_lower_than_short
+; GFX678: v_cvt_pk_i16_i32_e32 [[A:v[0-9]+]], [[A]], [[B:v[0-9]+]]
+; GFX9: v_cvt_pk_i16_i32 [[A:v[0-9]+]], [[A]], [[B:v[0-9]+]]
+; GFX6789: v_mov_b32_e32 [[B]], 0xffffff01
+; GFX6789: v_mov_b32_e32 [[C:v[0-9]+]], 0x100
+; GFX6789: v_med3_i32 [[A]], [[B]], [[A]], [[C]]
+; GFX10: v_cvt_pk_i16_i32_e64 [[A:v[0-9]+]], [[A]], [[B:v[0-9]+]]
+; GFX10: v_mov_b32_e32 [[C:v[0-9]+]], 0x100
+; GFX10: v_med3_i32 [[A]], 0xffffff01, [[A]], [[C]]
+define i16 @v_clamp_i64_i16_lower_than_short(i64 %in) #0 {
+entry:
+  %lower = icmp slt i64 %in, 256
+  %i1 = select i1 %lower, i64 %in, i64 256
+  %greater = icmp sgt i64 %i1, -255
+  %intermed = select i1 %greater, i64 %i1, i64 -255
+  %result = trunc i64 %intermed to i16
+  ret i16 %result
+}
+
+; GFX10-LABEL: {{^}}v_clamp_i64_i16_lower_than_short_reverse
+; GFX678: v_cvt_pk_i16_i32_e32 [[A:v[0-9]+]], [[A]], [[B:v[0-9]+]]
+; GFX9: v_cvt_pk_i16_i32 [[A:v[0-9]+]], [[A]], [[B:v[0-9]+]]
+; GFX6789: v_mov_b32_e32 [[B]], 0xffffff01
+; GFX6789: v_mov_b32_e32 [[C:v[0-9]+]], 0x100
+; GFX6789: v_med3_i32 [[A]], [[B]], [[A]], [[C]]
+; GFX10: v_cvt_pk_i16_i32_e64 [[A:v[0-9]+]], [[A]], [[B:v[0-9]+]]
+; GFX10: v_mov_b32_e32 [[C:v[0-9]+]], 0x100
+; GFX10: v_med3_i32 [[A]], 0xffffff01, [[A]], [[C]]
+define i16 @v_clamp_i64_i16_lower_than_short_reverse(i64 %in) #0 {
+entry:
+  %greater = icmp sgt i64 %in, -255
+  %i1 = select i1 %greater, i64 %in, i64 -255
+  %lower = icmp slt i64 %i1, 256
+  %intermed = select i1 %lower, i64 %i1, i64 256
+  %result = trunc i64 %intermed to i16
+  ret i16 %result
+}
+
+; GFX10-LABEL: {{^}}v_clamp_i64_i16_zero
+; GFX678: v_mov_b32_e32 [[A:v[0-9]+]], 0
+; GFX10: v_mov_b32_e32 [[A:v[0-9]+]], 0
+define i16 @v_clamp_i64_i16_zero(i64 %in) #0 {
+entry:
+  %greater = icmp sgt i64 %in, 0
+  %i1 = select i1 %greater, i64 %in, i64 0
+  %lower = icmp slt i64 %i1, 0
+  %intermed = select i1 %lower, i64 %i1, i64 0
+  %result = trunc i64 %intermed to i16
+  ret i16 %result
+}