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

[Jay Foad via Phabricator via llvm-commits]

foad 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
----------------
abinavpp wrote:
> 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?
Oh, my bad, I had forgotten about the known problem with cross regbank copies. No need to do anything now, let's just wait for a proper fix for that problem.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113448