[PATCH] D34291: [AMDGPU] Fix illegal shrink of V_SUBB_U32 and V_ADDC_U32

Matt Arsenault via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jun 19 10:58:52 PDT 2017


arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/SIShrinkInstructions.cpp:95
       case AMDGPU::V_SUBB_U32_e64:
+        if (TII->getNamedOperand(MI, AMDGPU::OpName::src1)->isImm())
+          return false;
----------------
rampitec wrote:
> arsenm wrote:
> > Can you add a test using a frame index and global address, those should have the same issue
> I really doubt they can directly come to either of these two instructions, and I'm probably unable to forge such a source. Then it I do it in mir...
> 
> ```
>         %vreg4<def>, %vreg5<def> = V_SUBBREV_U32_e64 <ga:@arr>, %vreg0, %vreg3, %EXEC<imp-use>; VGPR_32:%vreg4,%vreg0 SReg_64:%vreg5,%vreg3
> 
> *** Bad machine code: Illegal immediate value for operand. ***
> - function:    subbrev
> - basic block: BB#0  (0x58eb3e0)
> - instruction: %vreg4<def>, %vreg5<def> = V_SUBBREV_U32_e64
> ```
> 
> The placement in src0 is legal, that is global address is not legal for the operand, so it cannot even come to the pass I guess.
> 
> This is the OperandInfo:
> 
> ```
> static const MCOperandInfo OperandInfo243[] = { { AMDGPU::VGPR_32RegClassID, 0, MCOI::OPERAND_REGISTER, 0 }, { AMDGPU::SReg_64RegClassID, 0, MCOI::OPERAND_REGISTER, 0 }, { AMDGPU::VS_32RegClassID, 0, AMDGPU::OPERAND_REG_INLINE_C_INT32, 0 }, { AMDGPU::VS_32RegClassID, 0, AMDGPU::OPERAND_REG_INLINE_C_INT32, 0 }, { AMDGPU::SReg_64RegClassID, 0, AMDGPU::OPERAND_REG_INLINE_C_INT64, 0 }, };
> ```
> 
> verifyInstruction:
> 
> ```
>     case AMDGPU::OPERAND_REG_INLINE_C_INT32:
>     case AMDGPU::OPERAND_REG_INLINE_C_FP32:
>     case AMDGPU::OPERAND_REG_INLINE_C_INT64:
>     case AMDGPU::OPERAND_REG_INLINE_C_FP64:
>     case AMDGPU::OPERAND_REG_INLINE_C_INT16:
>     case AMDGPU::OPERAND_REG_INLINE_C_FP16: {
>       const MachineOperand &MO = MI.getOperand(i);
>       if (!MO.isReg() && (!MO.isImm() || !isInlineConstant(MI, i))) {
>         ErrInfo = "Illegal immediate value for operand.";
>         return false;
>       }
>       break;
> ```
> 
> So neither is expected as such an input.
Oh right, this is the inline constant case


================
Comment at: lib/Target/AMDGPU/SIShrinkInstructions.cpp:95
       case AMDGPU::V_SUBB_U32_e64:
+        if (TII->getNamedOperand(MI, AMDGPU::OpName::src1)->isImm())
+          return false;
----------------
arsenm wrote:
> rampitec wrote:
> > arsenm wrote:
> > > Can you add a test using a frame index and global address, those should have the same issue
> > I really doubt they can directly come to either of these two instructions, and I'm probably unable to forge such a source. Then it I do it in mir...
> > 
> > ```
> >         %vreg4<def>, %vreg5<def> = V_SUBBREV_U32_e64 <ga:@arr>, %vreg0, %vreg3, %EXEC<imp-use>; VGPR_32:%vreg4,%vreg0 SReg_64:%vreg5,%vreg3
> > 
> > *** Bad machine code: Illegal immediate value for operand. ***
> > - function:    subbrev
> > - basic block: BB#0  (0x58eb3e0)
> > - instruction: %vreg4<def>, %vreg5<def> = V_SUBBREV_U32_e64
> > ```
> > 
> > The placement in src0 is legal, that is global address is not legal for the operand, so it cannot even come to the pass I guess.
> > 
> > This is the OperandInfo:
> > 
> > ```
> > static const MCOperandInfo OperandInfo243[] = { { AMDGPU::VGPR_32RegClassID, 0, MCOI::OPERAND_REGISTER, 0 }, { AMDGPU::SReg_64RegClassID, 0, MCOI::OPERAND_REGISTER, 0 }, { AMDGPU::VS_32RegClassID, 0, AMDGPU::OPERAND_REG_INLINE_C_INT32, 0 }, { AMDGPU::VS_32RegClassID, 0, AMDGPU::OPERAND_REG_INLINE_C_INT32, 0 }, { AMDGPU::SReg_64RegClassID, 0, AMDGPU::OPERAND_REG_INLINE_C_INT64, 0 }, };
> > ```
> > 
> > verifyInstruction:
> > 
> > ```
> >     case AMDGPU::OPERAND_REG_INLINE_C_INT32:
> >     case AMDGPU::OPERAND_REG_INLINE_C_FP32:
> >     case AMDGPU::OPERAND_REG_INLINE_C_INT64:
> >     case AMDGPU::OPERAND_REG_INLINE_C_FP64:
> >     case AMDGPU::OPERAND_REG_INLINE_C_INT16:
> >     case AMDGPU::OPERAND_REG_INLINE_C_FP16: {
> >       const MachineOperand &MO = MI.getOperand(i);
> >       if (!MO.isReg() && (!MO.isImm() || !isInlineConstant(MI, i))) {
> >         ErrInfo = "Illegal immediate value for operand.";
> >         return false;
> >       }
> >       break;
> > ```
> > 
> > So neither is expected as such an input.
> Oh right, this is the inline constant case
This is the wrong place to handle this. This is exactly the same as the CNDMASK case, where this needs to be VCC. This should add the handling there so the regalloc hints are added so there is a better chance of shrinking in the post-RA run


================
Comment at: test/CodeGen/AMDGPU/shrink-carry.mir:6-7
+
+# GCN-LABEL: name: subb{{$}}
+# GCN:       V_SUBB_U32_e64 undef %vgpr0, 0, killed %vcc, implicit %exec
+
----------------
It's easier to read the test if these are put next to the function rather than all clustered at the top


Repository:
  rL LLVM

https://reviews.llvm.org/D34291





More information about the llvm-commits mailing list