AMDGPU: Fix getInstSizeInBytes

[nhaehnle]

Summary

Add some optional code to validate getInstSizeInBytes for emitted
instructions. This flushed out some issues which are fixed by this
patch:

• Streamline getInstSizeInBytes
• Properly define the VI readlane/writelane instruction as VOP3
• Fix the inline constant determination. Specifically, this change fixes an issue where a 32-bit value of 0xffffffff was recorded as unsigned. This is equal to -1 when restricting to a 32-bit comparison, and an inline constant can be used.

Change-Id: Id87c3b7975839da0de8156a124b0ce98c5fb47f2

[rampitec]

LGTM

[arsenm]

lib/Target/AMDGPU/AMDGPUMCInstLower.cpp

320 ↗

(On Diff #160320)

I strongly dislike assertion checks inside debug printing

320 ↗

(On Diff #160320)

Why can't you just assert after the instruction is emitted normally?

[nhaehnle]

Always do the sanity check in debug builds.

I was originally worried about how expensive this is, but it
doesn't show up in the running time of all the tests in
test/CodeGen/AMDGPU.

[arsenm]

lib/Target/AMDGPU/AMDGPUMCInstLower.cpp

320 ↗

(On Diff #160320)

You could always put this under an EXPENSIVE_TEST checks or whatever it's called, then at least the bots will be testing it

[arsenm]

In D50629#1209249, @nhaehnle wrote:

Always do the sanity check in debug builds.

I was originally worried about how expensive this is, but it
doesn't show up in the running time of all the tests in
test/CodeGen/AMDGPU.

These are by design almost all small, so that makes sense. It might not be representative in the real world

[arsenm]

lib/Target/AMDGPU/AMDGPUMCInstLower.cpp

308–314 ↗

(On Diff #161948)

We don't actually have a generic CPU? We just pick a default?

[nhaehnle]

In D50629#1209494, @arsenm wrote:

In D50629#1209249, @nhaehnle wrote:

Always do the sanity check in debug builds.

I was originally worried about how expensive this is, but it
doesn't show up in the running time of all the tests in
test/CodeGen/AMDGPU.

These are by design almost all small, so that makes sense. It might not be representative in the real world

We shouldn't be running debug builds in the real world anyway.

lib/Target/AMDGPU/AMDGPUMCInstLower.cpp

308–314 ↗

(On Diff #161948)

No, we don't. That's precisely the problem I mentioned elsewhere at some point and I brought up the possibility of just deciding that running without specifying a CPU is unsupported.

If you don't specify a CPU (or the CPU is unknown), you get a set of inconsistent feature bits that doesn't correspond to any real CPU. The consequence is that all sorts of MachineInstrs are generated during CodeGen which aren't actually considered supported by MC, so entering the instruction encoding logic will fail.

I briefly looked into ways of fixing this, and it's of course be possible, but it'd require fixing a *lot* of the existing tests.

[arsenm]

lib/Target/AMDGPU/AMDGPUMCInstLower.cpp

308–314 ↗

(On Diff #161948)

It looks like I changed this in r310258. I thought this was still defaulting to kaveri for HSA and tahiti for anything else

[nhaehnle]

lib/Target/AMDGPU/AMDGPUMCInstLower.cpp

308–314 ↗

(On Diff #161948)

We end up generating FLAT_LOAD_DWORD_ci without having FeatureCIInsts, for example.

[arsenm]

In D50629#1210582, @nhaehnle wrote:

In D50629#1209494, @arsenm wrote:

In D50629#1209249, @nhaehnle wrote:

Always do the sanity check in debug builds.

I was originally worried about how expensive this is, but it
doesn't show up in the running time of all the tests in
test/CodeGen/AMDGPU.

These are by design almost all small, so that makes sense. It might not be representative in the real world

We shouldn't be running debug builds in the real world anyway.

This will run with fast debug builds, which is what I usually use

[nhaehnle]

Guard sanity check with EXPENSIVE_CHECKS

[arsenm]

LGTM

llvm/trunk/lib/Target/AMDGPU/AMDGPUMCInstLower.cpp

if (MI->getOpcode() == AMDGPU::SI_MASKED_UNREACHABLE) {
         OutStreamer->emitRawComment(" divergent unreachable");
       return;
     }
 
     MCInst TmpInst;
     MCInstLowering.lower(MI, TmpInst);
     EmitToStreamer(*OutStreamer, TmpInst);
 
+#ifdef EXPENSIVE_CHECKS
+    // Sanity-check getInstSizeInBytes on explicitly specified CPUs (it cannot
+    // work correctly for the generic CPU).
+    //
+    // The isPseudo check really shouldn't be here, but unfortunately there are
+    // some negative lit tests that depend on being able to continue through
+    // here even when pseudo instructions haven't been lowered.
+    if (!MI->isPseudo() && STI.isCPUStringValid(STI.getCPU())) {
+      SmallVector<MCFixup, 4> Fixups;
+      SmallVector<char, 16> CodeBytes;
+      raw_svector_ostream CodeStream(CodeBytes);
+
+      std::unique_ptr<MCCodeEmitter> InstEmitter(createSIMCCodeEmitter(
+          *STI.getInstrInfo(), *OutContext.getRegisterInfo(), OutContext));
+      InstEmitter->encodeInstruction(TmpInst, CodeStream, Fixups, STI);
+
+      assert(CodeBytes.size() == STI.getInstrInfo()->getInstSizeInBytes(*MI));
+    }
+#endif
+
     if (STI.dumpCode()) {
       // Disassemble instruction/operands to text.
       DisasmLines.resize(DisasmLines.size() + 1);
       std::string &DisasmLine = DisasmLines.back();
       raw_string_ostream DisasmStream(DisasmLine);
 
       AMDGPUInstPrinter InstPrinter(*TM.getMCAsmInfo(),
                                     *STI.getInstrInfo(),

llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.cpp

bool SIInstrInfo::isInlineConstant(const MachineOperand &MO,
 
   int64_t Imm = MO.getImm();
   switch (OperandType) {
   case AMDGPU::OPERAND_REG_IMM_INT32:
   case AMDGPU::OPERAND_REG_IMM_FP32:
   case AMDGPU::OPERAND_REG_INLINE_C_INT32:
   case AMDGPU::OPERAND_REG_INLINE_C_FP32: {
     int32_t Trunc = static_cast<int32_t>(Imm);
-    return Trunc == Imm &&
-           AMDGPU::isInlinableLiteral32(Trunc, ST.hasInv2PiInlineImm());
+    return AMDGPU::isInlinableLiteral32(Trunc, ST.hasInv2PiInlineImm());
   }
   case AMDGPU::OPERAND_REG_IMM_INT64:
   case AMDGPU::OPERAND_REG_IMM_FP64:
   case AMDGPU::OPERAND_REG_INLINE_C_INT64:
   case AMDGPU::OPERAND_REG_INLINE_C_FP64:
     return AMDGPU::isInlinableLiteral64(MO.getImm(),
                                         ST.hasInv2PiInlineImm());
   case AMDGPU::OPERAND_REG_IMM_INT16:

 
 unsigned SIInstrInfo::getInstSizeInBytes(const MachineInstr &MI) const {
   unsigned Opc = MI.getOpcode();
   const MCInstrDesc &Desc = getMCOpcodeFromPseudo(Opc);
   unsigned DescSize = Desc.getSize();
 
   // If we have a definitive size, we can use it. Otherwise we need to inspect
   // the operands to know the size.
-  //
-  // FIXME: Instructions that have a base 32-bit encoding report their size as
-  // 4, even though they are really 8 bytes if they have a literal operand.
-  if (DescSize != 0 && DescSize != 4)
-    return DescSize;
-
   if (isFixedSize(MI))
     return DescSize;
 
   // 4-byte instructions may have a 32-bit literal encoded after them. Check
   // operands that coud ever be literals.
   if (isVALU(MI) || isSALU(MI)) {
     int Src0Idx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::src0);
     if (Src0Idx == -1)
-      return 4; // No operands.
+      return DescSize; // No operands.
 
     if (isLiteralConstantLike(MI.getOperand(Src0Idx), Desc.OpInfo[Src0Idx]))
-      return 8;
+      return DescSize + 4;
 
     int Src1Idx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::src1);
     if (Src1Idx == -1)
-      return 4;
+      return DescSize;
 
     if (isLiteralConstantLike(MI.getOperand(Src1Idx), Desc.OpInfo[Src1Idx]))
-      return 8;
+      return DescSize + 4;
 
-    return 4;
-  }
+    int Src2Idx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::src2);
+    if (Src2Idx == -1)
+      return DescSize;
 
-  if (DescSize == 4)
-    return 4;
+    if (isLiteralConstantLike(MI.getOperand(Src2Idx), Desc.OpInfo[Src2Idx]))
+      return DescSize + 4;
+
+    return DescSize;
+  }
 
   switch (Opc) {
   case TargetOpcode::IMPLICIT_DEF:
   case TargetOpcode::KILL:
   case TargetOpcode::DBG_VALUE:
   case TargetOpcode::EH_LABEL:
     return 0;
   case TargetOpcode::BUNDLE:
     return getInstBundleSize(MI);
   case TargetOpcode::INLINEASM: {
     const MachineFunction *MF = MI.getParent()->getParent();
     const char *AsmStr = MI.getOperand(0).getSymbolName();
     return getInlineAsmLength(AsmStr, *MF->getTarget().getMCAsmInfo());
   }
   default:
-    llvm_unreachable("unable to find instruction size");
+    return DescSize;
   }
 }
 
 bool SIInstrInfo::mayAccessFlatAddressSpace(const MachineInstr &MI) const {
   if (!isFLAT(MI))
     return false;
 
   if (MI.memoperands_empty())

llvm/trunk/lib/Target/AMDGPU/VOP2Instructions.td

class VOP2_DPP <bits<6> op, VOP2_Pseudo ps, string OpName = ps.OpName, VOPProfile P = ps.Pfl> :
   let Inst{16-9}  = !if(P.HasSrc1, src1{7-0}, 0);
   let Inst{24-17} = !if(P.EmitDst, vdst{7-0}, 0);
   let Inst{30-25} = op;
   let Inst{31}    = 0x0; //encoding
 }
 
 let AssemblerPredicates = [isVI], DecoderNamespace = "VI" in {
 
-multiclass VOP32_Real_vi <bits<10> op> {
-  def _vi :
-    VOP2_Real<!cast<VOP2_Pseudo>(NAME), SIEncodingFamily.VI>,
-    VOP3e_vi<op, !cast<VOP2_Pseudo>(NAME).Pfl>;
-}
-
 multiclass VOP2_Real_MADK_vi <bits<6> op> {
   def _vi : VOP2_Real<!cast<VOP2_Pseudo>(NAME), SIEncodingFamily.VI>,
             VOP2_MADKe<op{5-0}, !cast<VOP2_Pseudo>(NAME).Pfl>;
 }
 
 multiclass VOP2_Real_e32_vi <bits<6> op> {
   def _e32_vi :
     VOP2_Real<!cast<VOP2_Pseudo>(NAME#"_e32"), SIEncodingFamily.VI>,

 defm V_ADDC_CO_U32        : VOP2be_Real_e32e64_gfx9 <0x1c, "V_ADDC_U32",    "v_addc_co_u32">;
 defm V_SUBB_CO_U32        : VOP2be_Real_e32e64_gfx9 <0x1d, "V_SUBB_U32",    "v_subb_co_u32">;
 defm V_SUBBREV_CO_U32     : VOP2be_Real_e32e64_gfx9 <0x1e, "V_SUBBREV_U32", "v_subbrev_co_u32">;
 
 defm V_ADD_U32            : VOP2_Real_e32e64_gfx9 <0x34>;
 defm V_SUB_U32            : VOP2_Real_e32e64_gfx9 <0x35>;
 defm V_SUBREV_U32         : VOP2_Real_e32e64_gfx9 <0x36>;
 
-defm V_READLANE_B32       : VOP32_Real_vi <0x289>;
-defm V_WRITELANE_B32      : VOP32_Real_vi <0x28a>;
-
 defm V_BFM_B32            : VOP2_Real_e64only_vi <0x293>;
 defm V_BCNT_U32_B32       : VOP2_Real_e64only_vi <0x28b>;
 defm V_MBCNT_LO_U32_B32   : VOP2_Real_e64only_vi <0x28c>;
 defm V_MBCNT_HI_U32_B32   : VOP2_Real_e64only_vi <0x28d>;
 defm V_LDEXP_F32          : VOP2_Real_e64only_vi <0x288>;
 defm V_CVT_PKACCUM_U8_F32 : VOP2_Real_e64only_vi <0x1f0>;
 defm V_CVT_PKNORM_I16_F32 : VOP2_Real_e64only_vi <0x294>;
 defm V_CVT_PKNORM_U16_F32 : VOP2_Real_e64only_vi <0x295>;

llvm/trunk/lib/Target/AMDGPU/VOP3Instructions.td

 
 //===----------------------------------------------------------------------===//
 // VI
 //===----------------------------------------------------------------------===//
 
 let AssemblerPredicates = [isVI], DecoderNamespace = "VI" in {
 
 multiclass VOP3_Real_vi<bits<10> op> {
-  def _vi : VOP3_Real<!cast<VOP3_Pseudo>(NAME), SIEncodingFamily.VI>,
-            VOP3e_vi <op, !cast<VOP3_Pseudo>(NAME).Pfl>;
+  def _vi : VOP3_Real<!cast<VOP_Pseudo>(NAME), SIEncodingFamily.VI>,
+            VOP3e_vi <op, !cast<VOP_Pseudo>(NAME).Pfl>;
 }
 
 multiclass VOP3be_Real_vi<bits<10> op> {
-  def _vi : VOP3_Real<!cast<VOP3_Pseudo>(NAME), SIEncodingFamily.VI>,
-            VOP3be_vi <op, !cast<VOP3_Pseudo>(NAME).Pfl>;
+  def _vi : VOP3_Real<!cast<VOP_Pseudo>(NAME), SIEncodingFamily.VI>,
+            VOP3be_vi <op, !cast<VOP_Pseudo>(NAME).Pfl>;
 }
 
 multiclass VOP3OpSel_Real_gfx9<bits<10> op> {
-  def _vi : VOP3_Real<!cast<VOP3_Pseudo>(NAME), SIEncodingFamily.VI>,
-            VOP3OpSel_gfx9 <op, !cast<VOP3_Pseudo>(NAME).Pfl>;
+  def _vi : VOP3_Real<!cast<VOP_Pseudo>(NAME), SIEncodingFamily.VI>,
+            VOP3OpSel_gfx9 <op, !cast<VOP_Pseudo>(NAME).Pfl>;
 }
 
 multiclass VOP3Interp_Real_vi<bits<10> op> {
-  def _vi : VOP3_Real<!cast<VOP3_Pseudo>(NAME), SIEncodingFamily.VI>,
-            VOP3Interp_vi <op, !cast<VOP3_Pseudo>(NAME).Pfl>;
+  def _vi : VOP3_Real<!cast<VOP_Pseudo>(NAME), SIEncodingFamily.VI>,
+            VOP3Interp_vi <op, !cast<VOP_Pseudo>(NAME).Pfl>;
 }
 
 } // End AssemblerPredicates = [isVI], DecoderNamespace = "VI"
 
 let AssemblerPredicates = [isVIOnly], DecoderNamespace = "VI" in {
 
 multiclass VOP3_F16_Real_vi<bits<10> op> {
   def _vi : VOP3_Real<!cast<VOP3_Pseudo>(NAME), SIEncodingFamily.VI>,

 // removed from VI as identical to V_MUL_LO_U32
 let isAsmParserOnly = 1 in {
 defm V_MUL_LO_I32       : VOP3_Real_vi <0x285>;
 }
 
 defm V_MUL_HI_U32       : VOP3_Real_vi <0x286>;
 defm V_MUL_HI_I32       : VOP3_Real_vi <0x287>;
 
+defm V_READLANE_B32     : VOP3_Real_vi <0x289>;
+defm V_WRITELANE_B32    : VOP3_Real_vi <0x28a>;
+
 defm V_LSHLREV_B64      : VOP3_Real_vi <0x28f>;
 defm V_LSHRREV_B64      : VOP3_Real_vi <0x290>;
 defm V_ASHRREV_I64      : VOP3_Real_vi <0x291>;
 defm V_TRIG_PREOP_F64   : VOP3_Real_vi <0x292>;
 
 defm V_LSHL_ADD_U32 : VOP3_Real_vi <0x1fd>;
 defm V_ADD_LSHL_U32 : VOP3_Real_vi <0x1fe>;
 defm V_ADD3_U32 : VOP3_Real_vi <0x1ff>;

llvm/trunk/test/CodeGen/AMDGPU/llvm.amdgcn.writelane.ll

-; RUN: llc -mtriple=amdgcn--amdhsa -mcpu=tahiti -verify-machineinstrs < %s | FileCheck %s
+; RUN: llc -mtriple=amdgcn--amdhsa -mcpu=gfx700 -verify-machineinstrs < %s | FileCheck %s
 ; RUN: llc -mtriple=amdgcn--amdhsa -mcpu=gfx802 -verify-machineinstrs < %s | FileCheck %s
 
 declare i32 @llvm.amdgcn.writelane(i32, i32, i32) #0
 
 ; CHECK-LABEL: {{^}}test_writelane_sreg:
 ; CHECK: v_writelane_b32 v{{[0-9]+}}, s{{[0-9]+}}, s{{[0-9]+}}
 define amdgpu_kernel void @test_writelane_sreg(i32 addrspace(1)* %out, i32 %src0, i32 %src1) #1 {
   %oldval = load i32, i32 addrspace(1)* %out