[PATCH] D23399: [mips][ias] extend li macro when values are negated with ~

[Daniel Sanders via llvm-commits]

dsanders added a comment.

After discussing it with you off-list and experimenting with our gcc toolchain, I think you're on the right track. We need to accept the immediate if it is a simm33 and render it as a uimm32. This isn't quite the same as the 'relaxed' predicates so we should rename things as appropriate. This is slightly odd since `li` technically produces a signed value on the target but this doesn't affect the target behaviour.


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1025-1035
@@ -1024,2 +1024,13 @@
 
+  void addAnyImmOperands(MCInst &Inst, unsigned N) const {
+    assert(N == 1 && "Invalid number of operands!");
+    int64_t Val = getConstantImm();
+
+    // Note: this renderer truncates.
+    if (countLeadingOnes((uint64_t)Val) >= 32) {
+      Val &= 0xFFFFFFFF;
+    }
+    Inst.addOperand(MCOperand::createImm(Val));
+  }
+
   void addImmOperands(MCInst &Inst, unsigned N) const {
----------------
This seems to be nearly equivalent to addConstantUImmOperands<32>(). The difference is this only truncates if the value is within 32-bit whereas addConstantUImmOperands<32> always truncates. I'd expect out-of-range numbers to have been rejected by the predicate. I think we should use addConstantUImmOperands<32> instead.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:1100
@@ -1088,3 +1099,3 @@
   bool isConstantImm() const {
     return isImm() && isa<MCConstantExpr>(getImm());
   }
----------------
This predicate is wrong and I think it's a key reason (but not the only reason) you needed to add a new predicate and renderer. It currently evaluates to false for expressions like MCUnaryExpr('~', MCConstantExpr(0x7fffffff)) and can only be true for MCConstantExpr by itself. It should be calling evaluateAsAbsolute() and returning true if that's successful. getConstantImm() will need updating to match.

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:454
@@ +453,3 @@
+// Generic case - only to support certain assembly pseudo instructions.
+class UImmAnyAsmOperandClass<list<AsmOperandClass> Supers = []>
+    : AsmOperandClass {
----------------
This should probably take the immediate size for consistency (like SImmAsmOperandClass)

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:458
@@ +457,3 @@
+  let RenderMethod = "addAnyImmOperands";
+  let PredicateMethod = "isAnyInt<64>";
+  let SuperClasses = Supers;
----------------
I think this should be isSImm<33>

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:489
@@ +488,3 @@
+  let Name = "UImm32_Relaxed";
+  let PredicateMethod = "isAnyImm<64>";
+  let DiagnosticType = "UImm32_Relaxed";
----------------
This isn't needed, we can inherit it from UImmAnyAsmOperandClass

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:510-513
@@ -493,6 +509,6 @@
 }
 def UImm16AsmOperandClass
     : UImmAsmOperandClass<16, [UImm16RelaxedAsmOperandClass]>;
 def SImm16RelaxedAsmOperandClass
     : SImmAsmOperandClass<16, [UImm16RelaxedAsmOperandClass]> {
   let Name = "SImm16_Relaxed";
----------------
I've just noticed that one of these should probably be have UImm16AsmOperandClass as the superclass instead of UImm16RelaxedAsmOperandClass.


https://reviews.llvm.org/D23399