[PATCH] D24247: [SPARC] Add assembler for the REX instruction set extension

[James Y Knight via Phabricator via llvm-commits]

jyknight added inline comments.


================
Comment at: lib/Target/Sparc/AsmParser/SparcAsmParser.cpp:278
+
+  bool isMEMfi() const {
+    if (Kind != k_MemoryImm)
----------------
"fi" doesn't mean anything to me. I see what from the REX spec this is implementing, but it could use a better name, and a comment about what it's for.


================
Comment at: lib/Target/Sparc/AsmParser/SparcAsmParser.cpp:1386-1387
+      if (!Op.isRexFPReg())
+        break;
     case MCK_DFPRegs:
       if (!Op.isFloatReg() || SparcOperand::MorphToDoubleReg(Op))
----------------
Mark intended fall-through after code with LLVM_FALLTHROUGH


================
Comment at: lib/Target/Sparc/AsmParser/SparcAsmParser.cpp:1404-1405
+      if (!Op.isRexIntReg())
+        break;
+    case MCK_IntPair:
+      if (SparcOperand::MorphToIntPairReg(Op))
----------------
Same here.


================
Comment at: lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp:26
     llvm_unreachable("Unknown fixup kind!");
+  case Sparc::fixup_sparc_32:
+  case Sparc::fixup_sparc_disp32:
----------------
Why do we need this, vs using FK_Data_4?


================
Comment at: lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp:27
+  case Sparc::fixup_sparc_32:
+  case Sparc::fixup_sparc_disp32:
   case FK_Data_1:
----------------
And isn't this the same as FK_PCRel_4?


================
Comment at: lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp:206
+        { "fixup_sparc_32",             0,  32, 0 },
+        { "fixup_sparc_pc32",           0,  32, MCFixupKindInfo::FKF_IsPCRel },
+        { "fixup_sparc_br8",            0,   8, MCFixupKindInfo::FKF_IsPCRel },
----------------
You used a different name here (fixup_sparc_pc32 vs fixup_sparc_disp32).


================
Comment at: lib/Target/Sparc/MCTargetDesc/SparcMCCodeEmitter.cpp:192-193
   const MCOperand &MO = MI.getOperand(OpNo);
-  if (MO.isReg() || MO.isImm())
+  if (MO.isImm())
+    return MO.getImm() >> 2;
+  if (MO.isReg())
----------------
Why the change here?


================
Comment at: lib/Target/Sparc/MCTargetDesc/SparcMCCodeEmitter.cpp:244
+unsigned
+SparcMCCodeEmitter::getADDRfiOpValue(const MCInst &MI, unsigned OpNo,
+                                     SmallVectorImpl<MCFixup> &Fixups,
----------------
Should probably have Rex in its name.


================
Comment at: lib/Target/Sparc/Sparc.td:41
                      "Enable LEON extensions">;
+def FeatureREX
+  : SubtargetFeature<"rex", "IsREX", "true",
----------------
I think this needs to also be controlled by an AssemblerPredicate.

And we'll want to have many of the standard instructions (e.g. those that aren't format-3) with a predicate that disables them in rex mode too, right?


https://reviews.llvm.org/D24247