[PATCH] D113448: [AMDGPU] Check for unneeded shift mask in shift PatFrags.

[Abinav Puthan Purayil via Phabricator via llvm-commits]

abinavpp added inline comments.


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/fshl.ll:2927
 ; GFX10-NEXT:    v_xor_b32_e32 v3, -1, v2
+; GFX10-NEXT:    v_and_b32_e32 v2, 15, v2
 ; GFX10-NEXT:    v_lshrrev_b16 v1, 1, v1
----------------
foad wrote:
> @abinavpp Something has gone wrong here. These ANDs were removed by D110231, but now they have come back.
I was aware of this, but I don't think I made it clear in my previous comment:

> I'm not sure how the original PatFrags (i.e. the ones with the masks as literal
> constants without predicates) are working correctly in global-isel for *some*
> (vector cases and scalar 64-bit cases are not working) of the divergent cases.
> 

Sorry about that. The right way to fix this is to fix the cross regbank constant match problem in global-isel. A temporary workaround for this is to keep both the predicated and the literal match versions in PatFrags, i.e., to do:
```
--- a/llvm/lib/Target/AMDGPU/AMDGPUInstructions.td
+++ b/llvm/lib/Target/AMDGPU/AMDGPUInstructions.td
@@ -263,21 +263,24 @@ defvar mask = !sub(width, 1);
 defvar csh_mask = !cast<SDPatternOperator>("csh_mask_"#width);
 
 def cshl_#width : PatFrags<(ops node:$src0, node:$src1),
-  [(shl node:$src0, node:$src1), (shl node:$src0, (csh_mask node:$src1))]>;
+  [(shl node:$src0, node:$src1), (shl node:$src0, (csh_mask node:$src1)),
+    (shl node:$src0, (and node:$src1, mask))]>;
 defvar cshl = !cast<SDPatternOperator>("cshl_"#width);
 def cshl_#width#_oneuse : HasOneUseBinOp<cshl>;
 def clshl_rev_#width : PatFrag <(ops node:$src0, node:$src1),
   (cshl $src1, $src0)>;
 
 def csrl_#width : PatFrags<(ops node:$src0, node:$src1),
-  [(srl node:$src0, node:$src1), (srl node:$src0, (csh_mask node:$src1))]>;
+  [(srl node:$src0, node:$src1), (srl node:$src0, (csh_mask node:$src1)),
+    (srl node:$src0, (and node:$src1, mask))]>;
 defvar csrl = !cast<SDPatternOperator>("csrl_"#width);
 def csrl_#width#_oneuse : HasOneUseBinOp<csrl>;
 def clshr_rev_#width : PatFrag <(ops node:$src0, node:$src1),
   (csrl $src1, $src0)>;
 
 def csra_#width : PatFrags<(ops node:$src0, node:$src1),
-  [(sra node:$src0, node:$src1), (sra node:$src0, (csh_mask node:$src1))]>;
+  [(sra node:$src0, node:$src1), (sra node:$src0, (csh_mask node:$src1)),
+    (sra node:$src0, (and node:$src1, mask))]>;
```

Should I create a revision for the above change (for now) and then revert it after we fix the constant match problem in global-isel?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113448/new/

https://reviews.llvm.org/D113448