[PATCH] D100149: [AMDGPU][GlobalISel] Legalize and select G_SBFX and G_UBFX

Matt Arsenault via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu May 13 08:30:09 PDT 2021


arsenm added a comment.

LGTM with some nits. There are a few codesize regressions where a shift would work just as well but I'm not sure if there's anything you should be doing about it here



================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:3938-3940
+  // TODO: Enable non-constant values when all targets can legalize them.
+  if (!MI.getOperand(2).isImm())
+    return false;
----------------
Don't need to check this. It would be illegal mir to have a non-immediate here. All G_* opcode operands are always required to be the same operand kind


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:3955
+    auto Cst2 = B.buildConstant(ExtractTy, Width);
+    B.buildInstr(TargetOpcode::G_SBFX, {Dst}, {ShiftSrc, Cst1, Cst2});
+  };
----------------
Isn't there a buildSBfx? If not there should be


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:3989
+    auto Cst2 = B.buildConstant(ExtractTy, Width);
+    B.buildInstr(TargetOpcode::G_UBFX, {Dst}, {ShiftSrc, Cst1, Cst2});
+  };
----------------
Isn't there a buildUbfx? If not there should be


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:1523
+  Register SrcReg = MI.getOperand(FirstOpnd).getReg();
+  Register OffsetReg = MI.getOperand(FirstOpnd+1).getReg();
+  Register WidthReg = MI.getOperand(FirstOpnd+2).getReg();
----------------
Spaces around +


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:1524
+  Register OffsetReg = MI.getOperand(FirstOpnd+1).getReg();
+  Register WidthReg = MI.getOperand(FirstOpnd+2).getReg();
+
----------------
Spaces around +


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/fshl.ll:554
 ; GFX6-NEXT:    s_lshr_b32 s5, s5, 1
-; GFX6-NEXT:    s_lshr_b32 s1, s1, 9
+; GFX6-NEXT:    s_bfe_u32 s1, s1, 0x80008
 ; GFX6-NEXT:    s_lshr_b32 s2, s5, s2
----------------
This is worse for codesize


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/fshl.ll:676
 ; GFX6-NEXT:    v_lshrrev_b32_e32 v2, v2, v6
-; GFX6-NEXT:    v_lshrrev_b32_e32 v1, 9, v1
+; GFX6-NEXT:    v_bfe_u32 v1, v1, 8, 8
 ; GFX6-NEXT:    v_or_b32_e32 v0, v0, v2
----------------
This is worse for codesize. Is this just missing another simplify bits combine somewhere?


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-ubfx.mir:69
+---
+name:            test_ubfx_s8
+body: |
----------------
bcahoon wrote:
> arsenm wrote:
> > Should also test s16 which is more interesting. Also some vectors, in particular <2 x s32>, <2 x s16>, <3 x s16> and <4 x s16>
> Vector cases are disallowed for G_SBFX/G_UBFX by an explicit check in the MachineVerifier.
That doesn't make much sense but OK


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

https://reviews.llvm.org/D100149



More information about the llvm-commits mailing list