[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–158

Why the name change?

154

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

156

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

Indentation looks off

[artem.tamazov]

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

153–158

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

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

Single quotes for '<'

test/MC/AMDGPU/vop3.s

297–299

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

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.

lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp

     bool IsFPImm;
     ImmTy Type;
     int64_t Val;
+    int Modifiers;
   };
 
   struct RegOp {

   }
 
   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 {
-    Inst.addOperand(MCOperand::createImm(Reg.Modifiers));
-    addRegOperands(Inst, N);
+  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 {

     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;
+    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 {

     return Kind == Register && Reg.Modifiers == 0;
   }
 
-  bool isRegWithInputMods() const {
-    return Kind == Register;
+  bool isRegOrImmWithInputMods() const {
+    return Kind == Register || isInlinableImm();
   }
 
   bool isClamp() const {

   }
 
   void setModifiers(unsigned Mods) {
-    assert(isReg());
-    Reg.Modifiers = Mods;
+    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 {

   }
 
   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]
+    // How 64-bit values are formed from 32-bit literals in _B64 instructions?
+    return isImm() || isVCSrc64();
   }
 
   bool isMem() const override {

       OS << "<register " << getReg() << " mods: " << Reg.Modifiers << '>';
       break;
     case Immediate:
-      OS << getImm();
+      if (Imm.Type != AMDGPUOperand::ImmTyNone)
+        OS << getImm();
+      else 
+        OS << "<" << getImm() << " mods: " << Imm.Modifiers << '>';
       break;
     case Token:
       OS << '\'' << getToken() << '\'';

     Op->Imm.Val = Val;
     Op->Imm.IsFPImm = IsFPImm;
     Op->Imm.Type = Type;
+    Op->Imm.Modifiers = 0;
     Op->StartLoc = Loc;
     Op->EndLoc = Loc;
     return Op;

     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;

       // 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);
       }
     }

   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);
     }

lib/Target/AMDGPU/SIInstrInfo.td

 }
 
 def InputModsMatchClass : AsmOperandClass {
-  let Name = "RegWithInputMods";
+  let Name = "RegOrImmWithInputMods";
 }
 
 def InputModsNoDefault : Operand <i32> {

test/MC/AMDGPU/vop3-errs.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

test/MC/AMDGPU/vop3.s

 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]