This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Fix illegal shrink of V_SUBB_U32 and V_ADDC_U32
ClosedPublic

Authored by rampitec on Jun 16 2017, 1:23 PM.

Details

Reviewers
arsenm
vpykhtin
Summary

If there is an immediate operand we shall not shrink V_SUBB_U32 and V_ADDC_U32, it does not fit e32 encoding.

Diff Detail

Repository
rL LLVM

Event Timeline

rampitec created this revision.Jun 16 2017, 1:23 PM
rampitec updated this revision to Diff 102884.Jun 16 2017, 2:59 PM
rampitec retitled this revision from [AMDGPU] Fix illegal shrink of V_SUBB_U32 to [AMDGPU] Fix illegal shrink of V_SUBB_U32 and V_ADDC_U32.
rampitec edited the summary of this revision. (Show Details)

Same problem with V_ADDC_U32, was able to reproduce it with mir test.

arsenm added inline comments.Jun 16 2017, 4:16 PM
lib/Target/AMDGPU/SIShrinkInstructions.cpp
95

Can you add a test using a frame index and global address, those should have the same issue

rampitec added inline comments.Jun 16 2017, 4:46 PM
lib/Target/AMDGPU/SIShrinkInstructions.cpp
95

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.

arsenm added inline comments.Jun 19 2017, 10:58 AM
lib/Target/AMDGPU/SIShrinkInstructions.cpp
95

Oh right, this is the inline constant case

95

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

test/CodeGen/AMDGPU/shrink-carry.mir
6–7

It's easier to read the test if these are put next to the function rather than all clustered at the top

rampitec added inline comments.Jun 19 2017, 11:07 AM
lib/Target/AMDGPU/SIShrinkInstructions.cpp
95

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.

rampitec updated this revision to Diff 103080.Jun 19 2017, 11:11 AM
rampitec marked an inline comment as done.

Moved around check statements in the test.

test/CodeGen/AMDGPU/shrink-carry.mir
6–7

OK, To me it was easier to read when they are at the top, but not really very important.

arsenm added inline comments.Jun 19 2017, 11:21 AM
lib/Target/AMDGPU/SIShrinkInstructions.cpp
95

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

rampitec added inline comments.Jun 19 2017, 11:24 AM
lib/Target/AMDGPU/SIShrinkInstructions.cpp
95

Shrink pass runs before and after RA. That is after RA where it breaks, so hints will not help.

arsenm added inline comments.Jun 19 2017, 11:34 AM
lib/Target/AMDGPU/SIShrinkInstructions.cpp
95

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

rampitec added inline comments.Jun 19 2017, 11:36 AM
lib/Target/AMDGPU/SIShrinkInstructions.cpp
95

That is what I mean. I agree they are helpful, but that shall be a separate patch.

vpykhtin accepted this revision.Jun 20 2017, 7:42 AM

LGTM.

This revision is now accepted and ready to land.Jun 20 2017, 7:42 AM