[AMDGPU][llvm-mc] Support for 32-bit inline literals

[artem.tamazov]

Summary

Note: Support for 64-bit inline literals TBD
Added: Support of abs/neg modifiers for literals (incomplete; parsing TBD).
Added: Some TODO comments.
Reworked/clarity: rename isInlineImm() to isInlinableImm()
Reworked/robustness: disallow BitsToFloat() with undefined value in isInlinableImm()
Reworked/reuse: isSSrc32/64(), isVSrc32/64()
Tests added.

[arsenm]

Are source modifiers really allowed with immediates? The only case I can think of where that might be useful is for -0. There are a few assertions lying around that assume there are no modifiers for any immediate.

[arsenm]

Does this happen to fix this bug? : http://lists.llvm.org/pipermail/llvm-dev/2015-October/091715.html

If so there should be some tests for it.

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

153 ↗

(On Diff #47814)

Why the name change?

154 ↗

(On Diff #47814)

Comments should be capitalized. Also this looks like it goes over the 80 character line limit

156 ↗

(On Diff #47814)

We should avoid using host float here. It would be better to check the float bit values which is what a few other places do. We've had bot failures before due to weird NaN support on mips hosts.

209 ↗

(On Diff #47814)

Indentation looks off

[artem.tamazov]

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

153 ↗

(On Diff #47814)

If immediate is inline or not depends on how the actual instruction is encoded. The function only answers the question "is it possible to inline an immediate". I believe this name better reflects that.

156 ↗

(On Diff #47814)

That code was there before my change. I would prefer to fix it separately.

[artem.tamazov]

Comment capitalized. Indentation fixed.

[artem.tamazov]

In D17204#352929, @arsenm wrote:

Does this happen to fix this bug? : http://lists.llvm.org/pipermail/llvm-dev/2015-October/091715.html
If so there should be some tests for it.

I would prefer to do that separately. Please assign the bug to me, I will check, update the bug and submit regression tests. Is it OK?

[artem.tamazov]

In D17204#352927, @arsenm wrote:

Are source modifiers really allowed with immediates? The only case I can think of where that might be useful is for -0. There are a few assertions lying around that assume there are no modifiers for any immediate.

That's right, the only case which makes sense is -0.0. And it can be useful in some cases. I am going to double-check if modifiers are really allowed; that is the one of the reasons why I got pause in the middle of implementation and didn't implement parsing of modifiers for immediates.

[artem.tamazov]

In D17204#352979, @artem.tamazov wrote:

In D17204#352929, @arsenm wrote:

Does this happen to fix this bug? : http://lists.llvm.org/pipermail/llvm-dev/2015-October/091715.html
If so there should be some tests for it.

I would prefer to do that separately. Please assign the bug to me, I will check, update the bug and submit regression tests. Is it OK?

Oops, this is not a bugzilla item)) Well, let me handle that separately, okay?

[arsenm]

LGTM

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

296 ↗

(On Diff #48004)

Single quotes for '<'

test/MC/AMDGPU/vop3.s

297–299 ↗

(On Diff #48004)

Maybe add a test with the FP value specified as a hex constant?

[artem.tamazov]

Going to update diff with fresh llvm master and also to satisfy recent Matt's comments.

[artem.tamazov]

Added: Test for http://lists.llvm.org/pipermail/llvm-dev/2015-October/091715.html
Added: Test for FP value specified as a hex.
Added: Some TODO comments.
Updated/style: Some comments (80 chars line limit).
Updated/style: Single quotes for '<'.

[artem.tamazov]

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

156 ↗

(On Diff #48188)

TODO comment added

[artem.tamazov]

In D17204#352929, @arsenm wrote:

Does this happen to fix this bug? : http://lists.llvm.org/pipermail/llvm-dev/2015-October/091715.html
If so there should be some tests for it.

Yes, that bug is fixed. A test added.

[artem.tamazov]

In D17204#352927, @arsenm wrote:

Are source modifiers really allowed with immediates? The only case I can think of where that might be useful is for -0. There are a few assertions lying around that assume there are no modifiers for any immediate.

BTW Another useful case is negation of 1/(2*PI) inline literal.

llvm/trunk/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

struct TokOp {
     const char *Data;
     unsigned Length;
   };
 
   struct ImmOp {
     bool IsFPImm;
     ImmTy Type;
     int64_t Val;
+    int Modifiers;
   };
 
   struct RegOp {
     unsigned RegNo;
     int Modifiers;
     const MCRegisterInfo *TRI;
     const MCSubtargetInfo *STI;
     bool IsForcedVOP3;
StringRef getToken() const {
     return StringRef(Tok.Data, Tok.Length);
   }
 
   void addRegOperands(MCInst &Inst, unsigned N) const {
     Inst.addOperand(MCOperand::createReg(AMDGPU::getMCReg(getReg(), *Reg.STI)));
   }
 
   void addRegOrImmOperands(MCInst &Inst, unsigned N) const {
-    if (isReg())
+    if (isRegKind())
       addRegOperands(Inst, N);
     else
       addImmOperands(Inst, N);
   }
 
-  void addRegWithInputModsOperands(MCInst &Inst, unsigned N) const {
+  void addRegOrImmWithInputModsOperands(MCInst &Inst, unsigned N) const {
+    if (isRegKind()) {
       Inst.addOperand(MCOperand::createImm(Reg.Modifiers));
       addRegOperands(Inst, N);
+    } else {
+      Inst.addOperand(MCOperand::createImm(Imm.Modifiers));
+      addImmOperands(Inst, N);
+    }
   }
 
   void addSoppBrTargetOperands(MCInst &Inst, unsigned N) const {
     if (isImm())
       addImmOperands(Inst, N);
     else {
       assert(isExpr());
       Inst.addOperand(MCOperand::createExpr(Expr));
public:
   bool isToken() const override {
     return Kind == Token;
   }
 
   bool isImm() const override {
     return Kind == Immediate;
   }
 
-  bool isInlineImm() const {
-    float F = BitsToFloat(Imm.Val);
-    // TODO: Add 0.5pi for VI
-    return isImm() && ((Imm.Val <= 64 && Imm.Val >= -16) ||
+  bool isInlinableImm() const {
+    if (!isImm() || Imm.Type != AMDGPUOperand::ImmTyNone /* Only plain
+      immediates are inlinable (e.g. "clamp" attribute is not) */ )
+      return false;
+    // TODO: We should avoid using host float here. It would be better to
+    // check the float bit values which is what a few other places do. 
+    // We've had bot failures before due to weird NaN support on mips hosts.
+    const float F = BitsToFloat(Imm.Val);
+    // TODO: Add 1/(2*pi) for VI
+    return (Imm.Val <= 64 && Imm.Val >= -16) ||
            (F == 0.0 || F == 0.5 || F == -0.5 || F == 1.0 || F == -1.0 ||
-           F == 2.0 || F == -2.0 || F == 4.0 || F == -4.0));
+           F == 2.0 || F == -2.0 || F == 4.0 || F == -4.0);
   }
 
   bool isDSOffset0() const {
     assert(isImm());
     return Imm.Type == ImmTyDSOffset0;
   }
 
   bool isDSOffset1() const {
public:
   bool isRegKind() const {
     return Kind == Register;
   }
 
   bool isReg() const override {
     return Kind == Register && Reg.Modifiers == 0;
   }
 
-  bool isRegWithInputMods() const {
-    return Kind == Register;
+  bool isRegOrImmWithInputMods() const {
+    return Kind == Register || isInlinableImm();
   }
 
   bool isClamp() const {
     return isImm() && Imm.Type == ImmTyClamp;
   }
 
   bool isOMod() const {
     return isImm() && Imm.Type == ImmTyOMod;
   }
 
   bool isMod() const {
     return isClamp() || isOMod();
   }
 
   void setModifiers(unsigned Mods) {
-    assert(isReg());
+    assert(isReg() || (isImm() && Imm.Modifiers == 0));
+    if (isReg())
       Reg.Modifiers = Mods;
+    else
+      Imm.Modifiers = Mods;
   }
 
   bool hasModifiers() const {
-    assert(isRegKind());
-    return Reg.Modifiers != 0;
+    assert(isRegKind() || isImm());
+    return isRegKind() ? Reg.Modifiers != 0 : Imm.Modifiers != 0;
   }
 
   unsigned getReg() const override {
     return Reg.RegNo;
   }
 
   bool isRegOrImm() const {
     return isReg() || isImm();
   }
 
   bool isRegClass(unsigned RCID) const {
     return isReg() && Reg.TRI->getRegClass(RCID).contains(getReg());
   }
 
   bool isSCSrc32() const {
-    return isInlineImm() || (isReg() && isRegClass(AMDGPU::SReg_32RegClassID));
+    return isInlinableImm() || (isReg() && isRegClass(AMDGPU::SReg_32RegClassID));
   }
 
-  bool isSSrc32() const {
-    return isImm() || (isReg() && isRegClass(AMDGPU::SReg_32RegClassID));
+  bool isSCSrc64() const {
+    return isInlinableImm() || (isReg() && isRegClass(AMDGPU::SReg_64RegClassID));
   }
 
-  bool isSSrc64() const {
-    return isImm() || isInlineImm() ||
-           (isReg() && isRegClass(AMDGPU::SReg_64RegClassID));
+  bool isSSrc32() const {
+    return isImm() || isSCSrc32();
   }
 
-  bool isSCSrc64() const {
-    return (isReg() && isRegClass(AMDGPU::SReg_64RegClassID)) || isInlineImm();
+  bool isSSrc64() const {
+    // TODO: Find out how SALU supports extension of 32-bit literals to 64 bits.
+    // See isVSrc64().
+    return isImm() || isSCSrc64();
   }
 
   bool isVCSrc32() const {
-    return isInlineImm() || (isReg() && isRegClass(AMDGPU::VS_32RegClassID));
+    return isInlinableImm() || (isReg() && isRegClass(AMDGPU::VS_32RegClassID));
   }
 
   bool isVCSrc64() const {
-    return isInlineImm() || (isReg() && isRegClass(AMDGPU::VS_64RegClassID));
+    return isInlinableImm() || (isReg() && isRegClass(AMDGPU::VS_64RegClassID));
   }
 
   bool isVSrc32() const {
-    return isImm() || (isReg() && isRegClass(AMDGPU::VS_32RegClassID));
+    return isImm() || isVCSrc32();
   }
 
   bool isVSrc64() const {
-    return isImm() || (isReg() && isRegClass(AMDGPU::VS_64RegClassID));
+    // TODO: Check if the 64-bit value (coming from assembly source) can be 
+    // narrowed to 32 bits (in the instruction stream). That require knowledge
+    // of instruction type (unsigned/signed, floating or "untyped"/B64),
+    // see [AMD GCN3 ISA 6.3.1].
+    // TODO: How 64-bit values are formed from 32-bit literals in _B64 insns?
+    return isImm() || isVCSrc64();
   }
 
   bool isMem() const override {
     return false;
   }
 
   bool isExpr() const {
     return Kind == Expression;
public:
   }
 
   void print(raw_ostream &OS) const override {
     switch (Kind) {
     case Register:
       OS << "<register " << getReg() << " mods: " << Reg.Modifiers << '>';
       break;
     case Immediate:
+      if (Imm.Type != AMDGPUOperand::ImmTyNone)
         OS << getImm();
+      else 
+        OS << '<' << getImm() << " mods: " << Imm.Modifiers << '>';
       break;
     case Token:
       OS << '\'' << getToken() << '\'';
       break;
     case Expression:
       OS << "<expr " << *Expr << '>';
       break;
     }
   }
 
   static std::unique_ptr<AMDGPUOperand> CreateImm(int64_t Val, SMLoc Loc,
                                                   enum ImmTy Type = ImmTyNone,
                                                   bool IsFPImm = false) {
     auto Op = llvm::make_unique<AMDGPUOperand>(Immediate);
     Op->Imm.Val = Val;
     Op->Imm.IsFPImm = IsFPImm;
     Op->Imm.Type = Type;
+    Op->Imm.Modifiers = 0;
     Op->StartLoc = Loc;
     Op->EndLoc = Loc;
     return Op;
   }
 
   static std::unique_ptr<AMDGPUOperand> CreateToken(StringRef Str, SMLoc Loc,
                                            bool HasExplicitEncodingSize = true) {
     auto Res = llvm::make_unique<AMDGPUOperand>(Token);

 }
 
 static bool operandsHaveModifiers(const OperandVector &Operands) {
 
   for (unsigned i = 0, e = Operands.size(); i != e; ++i) {
     const AMDGPUOperand &Op = ((AMDGPUOperand&)*Operands[i]);
     if (Op.isRegKind() && Op.hasModifiers())
       return true;
+    if (Op.isImm() && Op.hasModifiers())
+      return true;
     if (Op.isImm() && (Op.getImmTy() == AMDGPUOperand::ImmTyOMod ||
                        Op.getImmTy() == AMDGPUOperand::ImmTyClamp))
       return true;
   }
   return false;
 }
 
 AMDGPUAsmParser::OperandMatchResultTy
if (HasModifiers || IsVOP3 ||
     AMDGPUAsmParser::OperandMatchResultTy Res =
         parseOptionalOps(VOP3OptionalOps, Operands);
 
     if (!HasModifiers && Res == MatchOperand_Success) {
       // We have added a modifier operation, so we need to make sure all
       // previous register operands have modifiers
       for (unsigned i = 2, e = Operands.size(); i != e; ++i) {
         AMDGPUOperand &Op = ((AMDGPUOperand&)*Operands[i]);
-        if (Op.isReg())
+        if ((Op.isReg() || Op.isImm()) && !Op.hasModifiers())
           Op.setModifiers(0);
       }
     }
     return Res;
   }
   return MatchOperand_NoMatch;
 }
 
void AMDGPUAsmParser::cvtVOP3(MCInst &Inst, const OperandVector &Operands) {
   const MCInstrDesc &Desc = MII.get(Inst.getOpcode());
   for (unsigned J = 0; J < Desc.getNumDefs(); ++J) {
     ((AMDGPUOperand &)*Operands[I++]).addRegOperands(Inst, 1);
   }
 
   unsigned ClampIdx = 0, OModIdx = 0;
   for (unsigned E = Operands.size(); I != E; ++I) {
     AMDGPUOperand &Op = ((AMDGPUOperand &)*Operands[I]);
-    if (Op.isRegWithInputMods()) {
-      Op.addRegWithInputModsOperands(Inst, 2);
+    if (Op.isRegOrImmWithInputMods()) {
+      Op.addRegOrImmWithInputModsOperands(Inst, 2);
     } else if (Op.isClamp()) {
       ClampIdx = I;
     } else if (Op.isOMod()) {
       OModIdx = I;
-    } else if (Op.isImm()) {
-      Op.addImmOperands(Inst, 1);
     } else {
       assert(false);
     }
   }
 
   if (ClampIdx) {
     AMDGPUOperand &Op = ((AMDGPUOperand &)*Operands[ClampIdx]);
     Op.addImmOperands(Inst, 1);

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

 //===----------------------------------------------------------------------===//
 
 // This must always be right before the operand being input modified.
 def InputMods : OperandWithDefaultOps <i32, (ops (i32 0))> {
   let PrintMethod = "printOperandAndMods";
 }
 
 def InputModsMatchClass : AsmOperandClass {
-  let Name = "RegWithInputMods";
+  let Name = "RegOrImmWithInputMods";
 }
 
 def InputModsNoDefault : Operand <i32> {
   let PrintMethod = "printOperandAndMods";
   let ParserMatchClass = InputModsMatchClass;
 }
 
 class getNumSrcArgs<ValueType Src0, ValueType Src1, ValueType Src2> {

llvm/trunk/test/MC/AMDGPU/vop3-errs.s

 // RUN: not llvm-mc -arch=amdgcn -show-encoding %s 2>&1 | FileCheck %s
 // RUN: not llvm-mc -arch=amdgcn -mcpu=SI -show-encoding %s 2>&1 | FileCheck %s
 
 v_add_f32_e64 v0, v1
 // CHECK: error: too few operands for instruction
+
+v_div_scale_f32  v24, vcc, v22, 1.1, v22
+// CHECK: error: invalid operand for instruction

llvm/trunk/test/MC/AMDGPU/vop3.s

 
 v_div_scale_f32  v24, vcc, s[10:11], v22, v20
 // SICI: v_div_scale_f32 v24, vcc, s[10:11], v22, v20 ; encoding: [0x18,0x6a,0xda,0xd2,0x0a,0x2c,0x52,0x04]
 // VI:   v_div_scale_f32 v24, vcc, s[10:11], v22, v20 ; encoding: [0x18,0x6a,0xe0,0xd1,0x0a,0x2c,0x52,0x04]
 
 v_div_scale_f32  v24, s[10:11], v22, v22, v20
 // SICI: v_div_scale_f32 v24, s[10:11], v22, v22, v20 ; encoding: [0x18,0x0a,0xda,0xd2,0x16,0x2d,0x52,0x04]
 // VI:   v_div_scale_f32 v24, s[10:11], v22, v22, v20 ; encoding: [0x18,0x0a,0xe0,0xd1,0x16,0x2d,0x52,0x04]
+
+v_div_scale_f32  v24, vcc, v22, 1.0, v22
+// SICI: v_div_scale_f32 v24, vcc, v22, 1.0, v22 ; encoding: [0x18,0x6a,0xda,0xd2,0x16,0xe5,0x59,0x04]
+// VI:   v_div_scale_f32 v24, vcc, v22, 1.0, v22 ; encoding: [0x18,0x6a,0xe0,0xd1,0x16,0xe5,0x59,0x04]
+
+v_div_scale_f32  v24, vcc, v22, v22, -2.0
+// SICI: v_div_scale_f32 v24, vcc, v22, v22, -2.0 ; encoding: [0x18,0x6a,0xda,0xd2,0x16,0x2d,0xd6,0x03]
+// VI:   v_div_scale_f32 v24, vcc, v22, v22, -2.0 ; encoding: [0x18,0x6a,0xe0,0xd1,0x16,0x2d,0xd6,0x03]
+
+v_div_scale_f32 v24, vcc, v22, v22, 0xc0000000
+// SICI: v_div_scale_f32 v24, vcc, v22, v22, -2.0 ; encoding: [0x18,0x6a,0xda,0xd2,0x16,0x2d,0xd6,0x03]
+// VI:   v_div_scale_f32 v24, vcc, v22, v22, -2.0 ; encoding: [0x18,0x6a,0xe0,0xd1,0x16,0x2d,0xd6,0x03]
+
+v_mad_f32 v9, 0.5, v5, -v8
+// SICI: v_mad_f32 v9, 0.5, v5, -v8      ; encoding: [0x09,0x00,0x82,0xd2,0xf0,0x0a,0x22,0x84]
+// VI:   v_mad_f32 v9, 0.5, v5, -v8      ; encoding: [0x09,0x00,0xc1,0xd1,0xf0,0x0a,0x22,0x84]