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

Stanislav Mekhanoshin via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jun 19 11:07:46 PDT 2017


rampitec 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;
----------------
arsenm wrote:
> 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
Not arguing about hints, but I do not see why shall we allow illegal shrinking.
A rock solid solution is to call isOperandLegal, but it needs an instruction already built. So that would mean to build an instruction, try, and delete it, which is way suboptimal.


Repository:
  rL LLVM

https://reviews.llvm.org/D34291





More information about the llvm-commits mailing list