AMDGPU : Add V_SAD_U32 instruction pattern and it's corresponding LIT tests.

[wdng]

Summary

AMDGPU : Add V_SAD_U32 instruction patterns and it's corresponding LIT tests.

[• tstellarAMD]

test/CodeGen/AMDGPU/sad.ll

19–20

I think we should also do a test case that selects from the result of the two sub operations, because I think this will give you different DagNodes. For example:

%sub0 = sub i32 %a, %b
%sub1 = sub i32 %b, %a
%ret0 = select i1 %icmp0, i32 %sub0, i32 %sub1

[wdng]

Add a second pattern for V_SAD_U32 based on Tom's comments.

[wdng]

Add a line break to reformat the V_SAD_U32 pattern.

[arsenm]

You should add more tests using i32 vectors, as well as i8/i16 types. The packed sads won't be matched yet, but you should still see the legalized to i32 select to this

test/CodeGen/AMDGPU/sad.ll

2

Remove the -mcpu=SI

5

This should be GCN. You should also check the operands

[arsenm]

You should also test when there are multiple uses of the components. You might want to add hasOneUse checks

[wdng]

In D23069#504384, @arsenm wrote:

You should also test when there are multiple uses of the components. You might want to add hasOneUse checks

Could you please say in more details about "add hasOneUse checks"? Thanks!

[• tstellarAMD]

In D23069#504384, @arsenm wrote:

You should also test when there are multiple uses of the components. You might want to add hasOneUse checks

Wouldn't you only want to not match the pattern when all the intermediate nodes had more than one use?

[arsenm]

In D23069#512440, @tstellarAMD wrote:

In D23069#504384, @arsenm wrote:

You should also test when there are multiple uses of the components. You might want to add hasOneUse checks

Wouldn't you only want to not match the pattern when all the intermediate nodes had more than one use?

For the first pattern, there are 4 instructions in the input (min, max, add sub), so
min or max multiple use -> min, sad
min and max multiple use -> min, max, sad
sub multiple use -> sub, min, max, sad

so I think the first pattern just needs a hasOneUse on the sub. The other one should be similar

lib/Target/AMDGPU/SIInstructions.td

3411–3413

I think you can remove most of the i32s. Only one should be sufficient

3411–3413

Probably just need to keep the ones on the named operands and not the results

[wdng]

1. Added LIT tests for i8, i16, i32 vector
2. Add hasOneUse() for V_SAD pattern and added multiple use of component LIT tests.
3. Remove unnecessary i32s from the V_SAD pattern.

[arsenm]

lib/Target/AMDGPU/AMDGPUInstructions.td

621

sub_su?

test/CodeGen/AMDGPU/sad.ll

35

The sad is not supposed to be emitted if the sub has multiple uses

43–49

It's easiest to add another use by storing the vale, rather than adding a lot more instructions

[wdng]

1. Renamed hasOneUse for sub instruction
2. Added a LIT test with no sad is generated.

[arsenm]

lib/Target/AMDGPU/SIInstructions.td

3417–3419

For this pattern the hasOneUse check belongs on the select.

test/CodeGen/AMDGPU/sad.ll

2

Space before <

71

You should end the label lines with the :, this prevents it from accidentally matching for something like max_pat12

105–111

It's simpler to use a store of a value instead of adding more complex uses for the multi use. It also helps to avoid adding instruction types similar to the ones you want to write checks for

[wdng]

1. Use store to replace multiple use of components.
2. Fix LIT test syntax issues.

[arsenm]

lib/Target/AMDGPU/SIInstructions.td

3417–3419

The hasOneUse belongs on the select here

test/CodeGen/AMDGPU/sad.ll

164–165

I don't think it's necessary to have multiple use checks for all of the vector versions

[wdng]

lib/Target/AMDGPU/SIInstructions.td

3417–3419

Why do we have to apply hasOneUse on "select" instead of "sub" for the second pattern?

[wdng]

1. Apply hasOneUse on select for the second pattern.
2. Remove checks for all of the vector versions.

[arsenm]

You should add some more tests with some constant operands. For the sub patterns, that may require looking for add of negative constants

lib/Target/AMDGPU/AMDGPUInstructions.td

628

select_oneuse should not go into the Properties block since it isn't commutative or associative

test/CodeGen/AMDGPU/sad.ll

102–115

There's no point to testing multiple uses of the final result

133

This is to specific for a not check. Not checks can be fragile especially when checking the operands. It would be better to check for the separate subs, select and add

[wdng]

This patch only adds constant test case for the first pattern. For the second pattern, it cannot handle it. As for an improvement, I will create a second patch for handling the constant case for the second pattern.

[wdng]

Changes based on Matt's last comments.

[wdng]

Removed one test which has multiple use of add for pattern 1.

[arsenm]

lib/Target/AMDGPU/SIInstructions.td

3418–3419

These operands should be indented to the same level as the other select operand to make it clear it's part of the select, and the add src2 should be on the next line after those

test/CodeGen/AMDGPU/sad.ll

42

You should ad another test where the compared values don't match, and another where the second sub uses a different value to make sure those aren't matched

[wdng]

test/CodeGen/AMDGPU/sad.ll

42

What do you mean the compared values don't match? If the second sub uses a different value, this is incorrect logic for the v_sad_u32 second pattern?

[wdng]

1. Add indentation for v_sad_u32 patterns for better code understanding.
2. Added one test where the compared values don't match, and another one where the second sub uses a different value to make sure those aren't matched.

[arsenm]

test/CodeGen/AMDGPU/sad.ll

154

This should be checking 4 times

170

This should be checking 4 times

[wdng]

Add check for 4 times.

[arsenm]

test/CodeGen/AMDGPU/sad.ll

191–193

These should not have zexts, the point of the test is what happens when it is legalized

211–213

Ditto

245–247

ditto, this should be like the previous test without the extensions

[wdng]

Remove zexts.

[arsenm]

LGTM

lib/Target/AMDGPU/AMDGPUInstructions.td

   [{ return N->hasOneUse(); }]
 >;
 
+class HasOneUseTernaryOp<SDPatternOperator op> : PatFrag<
+  (ops node:$src0, node:$src1, node:$src2),
+  (op $src0, $src1, $src2),
+  [{ return N->hasOneUse(); }]
+>;
+
 //===----------------------------------------------------------------------===//
 // Load/Store Pattern Fragments
 //===----------------------------------------------------------------------===//

 def smin_oneuse : HasOneUseBinOp<smin>;
 def umax_oneuse : HasOneUseBinOp<umax>;
 def umin_oneuse : HasOneUseBinOp<umin>;
+def sub_oneuse : HasOneUseBinOp<sub>;
 } // Properties = [SDNPCommutative, SDNPAssociative]
 
+def select_oneuse : HasOneUseTernaryOp<select>;
 
 // 24-bit arithmetic patterns
 def umul24 : PatFrag <(ops node:$x, node:$y), (mul node:$x, node:$y)>;

lib/Target/AMDGPU/SIInstructions.td

 defm : SI_INDIRECT_Pattern <v16i32, i32, "V16">;
 
 //===----------------------------------------------------------------------===//
+// SAD Patterns
+//===----------------------------------------------------------------------===//
+
+def : Pat <
+  (add (sub_oneuse (umax i32:$src0, i32:$src1),
+                   (umin i32:$src0, i32:$src1)),
+       i32:$src2),
+  (V_SAD_U32 $src0, $src1, $src2)
+>;
+
+def : Pat <
+  (add (select_oneuse (i1 (setugt i32:$src0, i32:$src1)),
+                      (sub i32:$src0, i32:$src1),
+                      (sub i32:$src1, i32:$src0)),
+       i32:$src2),
+  (V_SAD_U32 $src0, $src1, $src2)
+>;
+
+//===----------------------------------------------------------------------===//
 // Conversion Patterns
 //===----------------------------------------------------------------------===//
 

test/CodeGen/AMDGPU/sad.ll

+; RUN: llc -march=amdgcn -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
+
+; GCN-LABEL: {{^}}v_sad_u32_pat1:
+; GCN: v_sad_u32 v{{[0-9]+}}, s{{[0-9]+}}, v{{[0-9]+}}, v{{[0-9]+}}
+define void @v_sad_u32_pat1(i32 addrspace(1)* %out, i32 %a, i32 %b, i32 %c) {
+  %icmp0 = icmp ugt i32 %a, %b
+  %t0 = select i1 %icmp0, i32 %a, i32 %b
+
+  %icmp1 = icmp ule i32 %a, %b
+  %t1 = select i1 %icmp1, i32 %a, i32 %b
+
+  %ret0 = sub i32 %t0, %t1
+  %ret = add i32 %ret0, %c
+
+  store i32 %ret, i32 addrspace(1)* %out
+  ret void
+}
+
+; GCN-LABEL: {{^}}v_sad_u32_constant_pat1:
+; GCN: v_sad_u32 v{{[0-9]+}}, s{{[0-9]+}}, v{{[0-9]+}}, 20
+define void @v_sad_u32_constant_pat1(i32 addrspace(1)* %out, i32 %a) {
+  %icmp0 = icmp ugt i32 %a, 90
+  %t0 = select i1 %icmp0, i32 %a, i32 90
+
+  %icmp1 = icmp ule i32 %a, 90
+  %t1 = select i1 %icmp1, i32 %a, i32 90
+
+  %ret0 = sub i32 %t0, %t1
+  %ret = add i32 %ret0, 20
+
+  store i32 %ret, i32 addrspace(1)* %out
+  ret void
+}
+
+; GCN-LABEL: {{^}}v_sad_u32_pat2:
+; GCN: v_sad_u32 v{{[0-9]+}}, s{{[0-9]+}}, v{{[0-9]+}}, v{{[0-9]+}}
+define void @v_sad_u32_pat2(i32 addrspace(1)* %out, i32 %a, i32 %b, i32 %c) {
+  %icmp0 = icmp ugt i32 %a, %b
+  %sub0 = sub i32 %a, %b
+  %sub1 = sub i32 %b, %a
+  %ret0 = select i1 %icmp0, i32 %sub0, i32 %sub1
+
+  %ret = add i32 %ret0, %c
+
+  store i32 %ret, i32 addrspace(1)* %out
+  ret void
+}
+
+; GCN-LABEL: {{^}}v_sad_u32_multi_use_sub_pat1:
+; GCN: s_max_u32 s{{[0-9]+}}, s{{[0-9]+}}, s{{[0-9]+}}
+; GCN: s_min_u32 s{{[0-9]+}}, s{{[0-9]+}}, s{{[0-9]+}}
+; GCN: s_sub_i32 s{{[0-9]+}}, s{{[0-9]+}}, s{{[0-9]+}}
+; GCN: s_add_i32 s{{[0-9]+}}, s{{[0-9]+}}, s{{[0-9]+}}
+define void @v_sad_u32_multi_use_sub_pat1(i32 addrspace(1)* %out, i32 %a, i32 %b, i32 %c) {
+  %icmp0 = icmp ugt i32 %a, %b
+  %t0 = select i1 %icmp0, i32 %a, i32 %b
+
+  %icmp1 = icmp ule i32 %a, %b
+  %t1 = select i1 %icmp1, i32 %a, i32 %b
+
+  %ret0 = sub i32 %t0, %t1
+  store volatile i32 %ret0, i32 *undef
+  %ret = add i32 %ret0, %c
+
+  store i32 %ret, i32 addrspace(1)* %out
+  ret void
+}
+
+; GCN-LABEL: {{^}}v_sad_u32_multi_use_add_pat1:
+; GCN: v_sad_u32 v{{[0-9]+}}, s{{[0-9]+}}, v{{[0-9]+}}, v{{[0-9]+}}
+define void @v_sad_u32_multi_use_add_pat1(i32 addrspace(1)* %out, i32 %a, i32 %b, i32 %c) {
+  %icmp0 = icmp ugt i32 %a, %b
+  %t0 = select i1 %icmp0, i32 %a, i32 %b
+
+  %icmp1 = icmp ule i32 %a, %b
+  %t1 = select i1 %icmp1, i32 %a, i32 %b
+
+  %ret0 = sub i32 %t0, %t1
+  %ret = add i32 %ret0, %c
+  store volatile i32 %ret, i32 *undef
+  store i32 %ret, i32 addrspace(1)* %out
+  ret void
+}
+
+; GCN-LABEL: {{^}}v_sad_u32_multi_use_max_pat1:
+; GCN: v_sad_u32 v{{[0-9]+}}, s{{[0-9]+}}, v{{[0-9]+}}, v{{[0-9]+}}
+define void @v_sad_u32_multi_use_max_pat1(i32 addrspace(1)* %out, i32 %a, i32 %b, i32 %c) {
+  %icmp0 = icmp ugt i32 %a, %b
+  %t0 = select i1 %icmp0, i32 %a, i32 %b
+  store volatile i32 %t0, i32 *undef
+
+  %icmp1 = icmp ule i32 %a, %b
+  %t1 = select i1 %icmp1, i32 %a, i32 %b
+
+  %ret0 = sub i32 %t0, %t1
+  %ret = add i32 %ret0, %c
+
+  store i32 %ret, i32 addrspace(1)* %out
+  ret void
+}
+
+; GCN-LABEL: {{^}}v_sad_u32_multi_use_min_pat1:
+; GCN: v_sad_u32 v{{[0-9]+}}, s{{[0-9]+}}, v{{[0-9]+}}, v{{[0-9]+}}
+define void @v_sad_u32_multi_use_min_pat1(i32 addrspace(1)* %out, i32 %a, i32 %b, i32 %c) {
+  %icmp0 = icmp ugt i32 %a, %b
+  %t0 = select i1 %icmp0, i32 %a, i32 %b
+
+  %icmp1 = icmp ule i32 %a, %b
+  %t1 = select i1 %icmp1, i32 %a, i32 %b
+
+  store volatile i32 %t1, i32 *undef
+
+  %ret0 = sub i32 %t0, %t1
+  %ret = add i32 %ret0, %c
+
+  store i32 %ret, i32 addrspace(1)* %out
+  ret void
+}
+
+; GCN-LABEL: {{^}}v_sad_u32_multi_use_sub_pat2:
+; GCN: v_sad_u32 v{{[0-9]+}}, s{{[0-9]+}}, v{{[0-9]+}}, v{{[0-9]+}}
+define void @v_sad_u32_multi_use_sub_pat2(i32 addrspace(1)* %out, i32 %a, i32 %b, i32 %c) {
+  %icmp0 = icmp ugt i32 %a, %b
+  %sub0 = sub i32 %a, %b
+  store volatile i32 %sub0, i32 *undef
+  %sub1 = sub i32 %b, %a
+  %ret0 = select i1 %icmp0, i32 %sub0, i32 %sub1
+
+  %ret = add i32 %ret0, %c
+
+  store i32 %ret, i32 addrspace(1)* %out
+  ret void
+}
+
+; GCN-LABEL: {{^}}v_sad_u32_multi_use_select_pat2:
+; GCN: s_sub_i32 s{{[0-9]+}}, s{{[0-9]+}}, s{{[0-9]+}}
+; GCN: s_sub_i32 s{{[0-9]+}}, s{{[0-9]+}}, s{{[0-9]+}}
+; GCN: v_cmp_gt_u32_e32 vcc, s{{[0-9]+}}, v{{[0-9]+}}
+define void @v_sad_u32_multi_use_select_pat2(i32 addrspace(1)* %out, i32 %a, i32 %b, i32 %c) {
+  %icmp0 = icmp ugt i32 %a, %b
+  %sub0 = sub i32 %a, %b
+  %sub1 = sub i32 %b, %a
+  %ret0 = select i1 %icmp0, i32 %sub0, i32 %sub1
+  store volatile i32 %ret0, i32 *undef
+
+  %ret = add i32 %ret0, %c
+
+  store i32 %ret, i32 addrspace(1)* %out
+  ret void
+}
+
+; GCN-LABEL: {{^}}v_sad_u32_vector_pat1:
+; GCN: v_sad_u32 v{{[0-9]+}}, s{{[0-9]+}}, v{{[0-9]+}}, v{{[0-9]+}}
+; GCN: v_sad_u32 v{{[0-9]+}}, s{{[0-9]+}}, v{{[0-9]+}}, v{{[0-9]+}}
+; GCN: v_sad_u32 v{{[0-9]+}}, s{{[0-9]+}}, v{{[0-9]+}}, v{{[0-9]+}}
+; GCN: v_sad_u32 v{{[0-9]+}}, s{{[0-9]+}}, v{{[0-9]+}}, v{{[0-9]+}}
+define void @v_sad_u32_vector_pat1(<4 x i32> addrspace(1)* %out, <4 x i32> %a, <4 x i32> %b, <4 x i32> %c) {
+  %icmp0 = icmp ugt <4 x i32> %a, %b
+  %t0 = select <4 x i1> %icmp0, <4 x i32> %a, <4 x i32> %b
+
+  %icmp1 = icmp ule <4 x i32> %a, %b
+  %t1 = select <4 x i1> %icmp1, <4 x i32> %a, <4 x i32> %b
+
+  %ret0 = sub <4 x i32> %t0, %t1
+  %ret = add <4 x i32> %ret0, %c
+
+  store <4 x i32> %ret, <4 x i32> addrspace(1)* %out
+  ret void
+}
+
+; GCN-LABEL: {{^}}v_sad_u32_vector_pat2:
+; GCN: v_sad_u32 v{{[0-9]+}}, s{{[0-9]+}}, v{{[0-9]+}}, v{{[0-9]+}}
+; GCN: v_sad_u32 v{{[0-9]+}}, s{{[0-9]+}}, v{{[0-9]+}}, v{{[0-9]+}}
+; GCN: v_sad_u32 v{{[0-9]+}}, s{{[0-9]+}}, v{{[0-9]+}}, v{{[0-9]+}}
+; GCN: v_sad_u32 v{{[0-9]+}}, s{{[0-9]+}}, v{{[0-9]+}}, v{{[0-9]+}}
+define void @v_sad_u32_vector_pat2(<4 x i32> addrspace(1)* %out, <4 x i32> %a, <4 x i32> %b, <4 x i32> %c) {
+  %icmp0 = icmp ugt <4 x i32> %a, %b
+  %sub0 = sub <4 x i32> %a, %b
+  %sub1 = sub <4 x i32> %b, %a
+  %ret0 = select <4 x i1> %icmp0, <4 x i32> %sub0, <4 x i32> %sub1
+
+  %ret = add <4 x i32> %ret0, %c
+
+  store <4 x i32> %ret, <4 x i32> addrspace(1)* %out
+  ret void
+}
+
+; GCN-LABEL: {{^}}v_sad_u32_i16_pat1:
+; GCN: v_sad_u32 v{{[0-9]+}}, s{{[0-9]+}}, v{{[0-9]+}}, v{{[0-9]+}}
+define void @v_sad_u32_i16_pat1(i32 addrspace(1)* %out, i16 %a, i16 %b, i16 %c) {
+  %a.ext = zext i16 %a to i32
+  %b.ext = zext i16 %b to i32
+  %c.ext = zext i16 %c to i32
+
+  %icmp0 = icmp ugt i32 %a.ext, %b.ext
+  %t0 = select i1 %icmp0, i32 %a.ext, i32 %b.ext
+
+  %icmp1 = icmp ule i32 %a.ext, %b.ext
+  %t1 = select i1 %icmp1, i32 %a.ext, i32 %b.ext
+
+  %ret0 = sub i32 %t0, %t1
+  %ret = add i32 %ret0, %c.ext
+
+  store i32 %ret, i32 addrspace(1)* %out
+  ret void
+}
+
+; GCN-LABEL: {{^}}v_sad_u32_i16_pat2:
+; GCN: v_sad_u32 v{{[0-9]+}}, s{{[0-9]+}}, v{{[0-9]+}}, v{{[0-9]+}}
+define void @v_sad_u32_i16_pat2(i32 addrspace(1)* %out, i16 %a, i16 %b, i16 %c) {
+  %a.ext = zext i16 %a to i32
+  %b.ext = zext i16 %b to i32
+  %c.ext = zext i16 %c to i32
+
+  %icmp0 = icmp ugt i32 %a.ext, %b.ext
+  %sub0 = sub i32 %a.ext, %b.ext
+  %sub1 = sub i32 %b.ext, %a.ext
+  %ret0 = select i1 %icmp0, i32 %sub0, i32 %sub1
+
+  %ret = add i32 %ret0, %c.ext
+
+  store i32 %ret, i32 addrspace(1)* %out
+  ret void
+}
+
+; GCN-LABEL: {{^}}v_sad_u32_i8_pat1:
+; GCN: v_sad_u32 v{{[0-9]+}}, s{{[0-9]+}}, v{{[0-9]+}}, v{{[0-9]+}}
+define void @v_sad_u32_i8_pat1(i8 addrspace(1)* %out, i8 %a, i8 %b, i8 %c) {
+  %icmp0 = icmp ugt i8 %a, %b
+  %t0 = select i1 %icmp0, i8 %a, i8 %b
+
+  %icmp1 = icmp ule i8 %a, %b
+  %t1 = select i1 %icmp1, i8 %a, i8 %b
+
+  %ret0 = sub i8 %t0, %t1
+  %ret = add i8 %ret0, %c
+
+  store i8 %ret, i8 addrspace(1)* %out
+  ret void
+}
+
+; GCN-LABEL: {{^}}v_sad_u32_i8_pat2:
+; GCN: v_sad_u32 v{{[0-9]+}}, s{{[0-9]+}}, v{{[0-9]+}}, v{{[0-9]+}}
+define void @v_sad_u32_i8_pat2(i32 addrspace(1)* %out, i8 %a, i8 %b, i8 %c) {
+  %a.ext = zext i8 %a to i32
+  %b.ext = zext i8 %b to i32
+  %c.ext = zext i8 %c to i32
+
+  %icmp0 = icmp ugt i32 %a.ext, %b.ext
+  %sub0 = sub i32 %a.ext, %b.ext
+  %sub1 = sub i32 %b.ext, %a.ext
+  %ret0 = select i1 %icmp0, i32 %sub0, i32 %sub1
+
+  %ret = add i32 %ret0, %c.ext
+
+  store i32 %ret, i32 addrspace(1)* %out
+  ret void
+}
+
+; GCN-LABEL: {{^}}v_sad_u32_mismatched_operands_pat1:
+; GCN: v_cmp_le_u32_e32 vcc, s{{[0-9]+}}, v{{[0-9]+}}
+; GCN: s_max_u32 s{{[0-9]+}}, s{{[0-9]+}}, s{{[0-9]+}}
+; GCN: v_sub_i32_e32 v{{[0-9]+}}, vcc, s{{[0-9]+}}, v{{[0-9]+}}
+; GCN: v_add_i32_e32 v{{[0-9]+}}, vcc, s{{[0-9]+}}, v{{[0-9]+}}
+define void @v_sad_u32_mismatched_operands_pat1(i32 addrspace(1)* %out, i32 %a, i32 %b, i32 %c, i32 %d) {
+  %icmp0 = icmp ugt i32 %a, %b
+  %t0 = select i1 %icmp0, i32 %a, i32 %b
+
+  %icmp1 = icmp ule i32 %a, %b
+  %t1 = select i1 %icmp1, i32 %a, i32 %d
+
+  %ret0 = sub i32 %t0, %t1
+  %ret = add i32 %ret0, %c
+
+  store i32 %ret, i32 addrspace(1)* %out
+  ret void
+}
+
+; GCN-LABEL: {{^}}v_sad_u32_mismatched_operands_pat2:
+; GCN: s_sub_i32 s{{[0-9]+}}, s{{[0-9]+}}, s{{[0-9]+}}
+; GCN: s_sub_i32 s{{[0-9]+}}, s{{[0-9]+}}, s{{[0-9]+}}
+; GCN: v_add_i32_e32 v{{[0-9]+}}, vcc, s{{[0-9]+}}, v{{[0-9]+}}
+define void @v_sad_u32_mismatched_operands_pat2(i32 addrspace(1)* %out, i32 %a, i32 %b, i32 %c, i32 %d) {
+  %icmp0 = icmp ugt i32 %a, %b
+  %sub0 = sub i32 %a, %d
+  %sub1 = sub i32 %b, %a
+  %ret0 = select i1 %icmp0, i32 %sub0, i32 %sub1
+
+  %ret = add i32 %ret0, %c
+
+  store i32 %ret, i32 addrspace(1)* %out
+  ret void
+}
+