[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 11:34:20 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:
> > rampitec wrote:
> > > 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.
> > It's not allowing illegal shrinking. It just defers the VCC specific check later to apply the register allocator hints and some other reason I forgot. canShrink could use a better name
> Shrink pass runs before and after RA. That is after RA where it breaks, so hints will not help.
The hints aren't supposed to fix the problem, they are supposed to increase the chance shrinking will be possible in the post-RA run


Repository:
  rL LLVM

https://reviews.llvm.org/D34291





More information about the llvm-commits mailing list