[AMDGPU] Add BFE pattern matches for constrained shifts.

[Pierre-vh]

Summary

042e0883cbcd35641d60fd2d22105ac5c6a402f8 made clang conform to
https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_C.html#operators-shift

So, if a clang -x c ... emits:

%sub = sub i32 32, %bits
%shl = shl i32 %a, %sub

The equivalent clang -x cl ... after optimization can emit:

%sub = sub i32 0, %bits
%shl.mask = and i32 %sub, 31
%shl = shl i32 %a, %shl.mask

This change adds the BFE pattern match for the -x cl output.

[foad]

This will give the wrong result when $width is zero.

[arsenm]

Can we more generally just fold out and of 31 since the shifts ignore the high bits anyway?

[arsenm]

In D109671#2996887, @foad wrote:

This will give the wrong result when $width is zero.

I'm not seeing the problem with 0?

[abinavpp]

In D109671#2997988, @arsenm wrote:

In D109671#2996887, @foad wrote:

This will give the wrong result when $width is zero.

I'm not seeing the problem with 0?

The introduced (srl (shl $src, (and (sub 0, $width), 31)), (and (sub 0, $width), 31))
on $width = 0 evaluates to (srl (shl $src, 0), 0) = $src != (v_bfe_u32 $src, 0, 0)

The original (srl (shl $src, (sub 32, $width)), (sub 32, $width)) on $width = 0
will evaluate to (srl (shl $src, 32), 32). Assuming the ISD::SHL's semantics is
the same as that of LLVM-IR's shl, (shl $src, 32) yields a poison value. If I
understand correctly, selecting v_bfe_u32 here is acceptable but not for the
pattern introduced in this revision.

[abinavpp]

I think we need the constrained shift pattern to be of the form (srl (shl $src,
(and (sub 32, $width), 31)), (and (sub 32, $width), 31)), which, on $width = 0,
will evaluate to (srl (shl $src, 32), 32) like the original pattern.

I wrote the (sub 0, $width) version since inst-combine, on visiting (and (sub
32, $width), 31), transforms the sub using SimplifyDemandedInstructionBits()
to (sub 0, $width). You can see this in the compilation of
https://github.com/RadeonOpenCompute/ROCm/issues/989 's test.

I'm not sure if there's a reasonable way for inst-combine's visitAnd() to stop
doing this so that we can match the BFE pattern correctly.

[abinavpp]

In D109671#2997977, @arsenm wrote:

Can we more generally just fold out and of 31 since the shifts ignore the high bits anyway?

Did you mean https://alive2.llvm.org/ce/z/wE_G9a ?

[foad]

In D109671#2997988, @arsenm wrote:

In D109671#2996887, @foad wrote:

This will give the wrong result when $width is zero.

I'm not seeing the problem with 0?

If $width is zero then the patterns being matched return $src unchanged, but the V_BFE instruction returns zero.

[abinavpp]

In D109671#2999083, @abinavpp wrote:

In D109671#2997977, @arsenm wrote:

Can we more generally just fold out and of 31 since the shifts ignore the high bits anyway?

Did you mean https://alive2.llvm.org/ce/z/wE_G9a ?

I guess you meant https://reviews.llvm.org/D110231

[Pierre-vh]

I'm going to try and finish this one. @foad, @arsenm do you remember what needed to be done here to get this in a land-able state?

[foad]

In D109671#4095742, @Pierre-vh wrote:

I'm going to try and finish this one. @foad, @arsenm do you remember what needed to be done here to get this in a land-able state?

I don't remember any more than what I wrote in previous comments.

llvm/lib/Target/AMDGPU/SIInstructions.td

 >;
 
 def : AMDGPUPat <
   (DivergentBinFrag<sra> (shl_oneuse i32:$src, (sub 32, i32:$width)),
                          (sub 32, i32:$width)),
   (V_BFE_I32_e64 $src, (i32 0), $width)
 >;
 
+// An OpenCL front-end, as per
+// https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_C.html#operators-shift
+// , can emit (and (sub 32, i32:$x) 31) instead of (sub 32, i32:$x) as the
+// second operand of a 32 bit shift expression. This operand can be transformed
+// to (and (sub 0, i32:$x), 31) by the optimizer.
+def : AMDGPUPat <
+  (DivergentBinFrag<srl> (shl_oneuse i32:$src, (and (sub 0, i32:$width), 31)),
+                         (and (sub 0, i32:$width), 31)),
+  (V_BFE_U32_e64 $src, (i32 0), $width)
+>;
+
+def : AMDGPUPat <
+  (DivergentBinFrag<sra> (shl_oneuse i32:$src, (and (sub 0, i32:$width), 31)),
+                         (and (sub 0, i32:$width), 31)),
+  (V_BFE_I32_e64 $src, (i32 0), $width)
+>;
+
 // SHA-256 Ma patterns
 
 // ((x & z) | (y & (x | z))) -> BFI (XOR x, y), z, y
 def : AMDGPUPat <
   (DivergentBinFrag<or> (and i32:$x, i32:$z),
                         (and i32:$y, (or i32:$x, i32:$z))),
   (V_BFI_B32_e64 (V_XOR_B32_e64 i32:$x, i32:$y), i32:$z, i32:$y)
 >;

llvm/test/CodeGen/AMDGPU/bfe-patterns.ll

define amdgpu_kernel void @v_ubfe_sub_i32(i32 addrspace(1)* %out, i32 addrspace(1)* %in0, i32 addrspace(1)* %in1) #1 {
   %width = load volatile i32, i32 addrspace(1)* %in0.gep
   %sub = sub i32 32, %width
   %shl = shl i32 %src, %sub
   %bfe = lshr i32 %shl, %sub
   store i32 %bfe, i32 addrspace(1)* %out.gep
   ret void
 }
 
+; GCN-LABEL: {{^}}shl_mask:
+; GCN: v_bfe_i32 v0, v0, 0, v1
+define i32 @shl_mask(i32 %a, i32 %bits) {
+  %sub = sub i32 0, %bits
+  %shl.mask = and i32 %sub, 31
+  %shl = shl i32 %a, %shl.mask
+  %shr = ashr i32 %shl, %shl.mask
+  ret i32 %shr
+}
+
 ; GCN-LABEL: {{^}}v_ubfe_sub_multi_use_shl_i32:
 ; GCN: {{buffer|flat}}_load_dword [[SRC:v[0-9]+]]
 ; GCN: {{buffer|flat}}_load_dword [[WIDTH:v[0-9]+]]
 ; GCN: v_sub_{{[iu]}}32_e32 [[SUB:v[0-9]+]], vcc, 32, [[WIDTH]]
 
 ; SI-NEXT: v_lshl_b32_e32 [[SHL:v[0-9]+]], [[SRC]], [[SUB]]
 ; SI-NEXT: v_lshr_b32_e32 [[BFE:v[0-9]+]], [[SHL]], [[SUB]]