[PATCH] D11675: [mips] Added support for the div, divu, ddiv and ddivu macros using traps and breaks in the integrated assembler.

[Daniel Sanders via llvm-commits]

dsanders added a comment.

Mostly a few style and spelling nits.

Regarding the magic numbers for branch targets: I definitely want to eliminate these (they are unlikely to be correct for microMIPS) but I can't think of a way to do so in the current implementation for macros. The problem is that we need to drop private labels at a future point in the instruction stream but we can't arrange this since we are buffering the instructions before emitting them all at once. We need to move to a mechanism where we emit the instructions directly as we build them. For now, could you make them named constants and mark them with a FIXME?


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2581
@@ +2580,3 @@
+                              const bool IsMips64, const bool Signed) {
+  if (hasMips32r6() || hasMips64r6()) {
+    Error(IDLoc, "instruction not supported on mips32r6 or mips64r6");
----------------
The '|| hasMips64r6()' is redundant. The predicates are cumulative.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2613-2617
@@ +2612,7 @@
+      if (Signed && (RtReg == Mips::ZERO || RtReg == Mips::ZERO_64)) {
+        if (UseTraps)
+          emitRRI(Mips::TEQ, RtReg, ZeroReg, 0x7, IDLoc, Instructions);
+        else
+          emitII(Mips::BREAK, 0x7, 0, IDLoc, Instructions);
+        return false;
+      }
----------------
Style nit:

  if (UseTraps) {
    emitRRI(...);
    return false;
  }

  emitII(...);
  return false;

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2619
@@ +2618,3 @@
+      }
+    } else {
+      emitRR(DivOp, RsReg, RtReg, IDLoc, Instructions);
----------------
Style nit: Avoid else after return

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2628-2632
@@ +2627,7 @@
+    if (Signed) {
+      if (UseTraps)
+        emitRRI(Mips::TEQ, RtReg, ZeroReg, 0x7, IDLoc, Instructions);
+      else
+        emitII(Mips::BREAK, 0x7, 0, IDLoc, Instructions);
+      return false;
+    }
----------------
Style nit:
  if (UseTraps) {
    emitRRI(...);
    return false;
  }

  emitII(...);
  return false;

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2639
@@ +2638,3 @@
+  else
+    emitRRI(Mips::BNE, RtReg, ZeroReg, 2*4, IDLoc, Instructions); //2*4 because it needs to branch to the li instruction
+
----------------
Magic number for offset: 2*4

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2657
@@ +2656,3 @@
+  if (IsMips64) {
+    emitRRI(Mips::BNE, RtReg, ATReg, UseTraps ? 3*4 : 5*4, IDLoc, Instructions); //3*4 or 5*4 because it needs to branch to the mflo instruction
+    emitRRI(Mips::ADDiu, ATReg, ZeroReg, 1, IDLoc, Instructions);
----------------
Magic number for offset: 3*4
Magic number for offset: 5*4

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2661
@@ +2660,3 @@
+  } else {
+    emitRRI(Mips::BNE, RtReg, ATReg, UseTraps ? 2*4 : 4*4, IDLoc, Instructions); //2*4 or 4*4 because it needs to branch to the mflo instruction
+    emitRI(Mips::LUi, ATReg, (uint16_t)0x8000, IDLoc, Instructions);
----------------
Magic number for offset: 2*4
Magic number for offset: 4*4

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2668
@@ +2667,3 @@
+  else {
+    emitRRI(Mips::BNE, RsReg, ATReg, 2*4, IDLoc, Instructions); //2*4 because it needs to branch to the mflo instruction
+    emitRRI(Mips::SLL, ZeroReg, ZeroReg, 0, IDLoc, Instructions);
----------------
Magic number for offset: 2*4

================
Comment at: lib/Target/Mips/Mips.td:166-168
@@ -165,2 +165,5 @@
                                 [FeatureMips64r2]>;
+def FeatureUseDivTraps : SubtargetFeature<"use-div-traps",
+                               "UseDivTraps", "false",
+                               "Force the assembler to use trapping">;
 
----------------
The names and strings here are a little confusing.

In both cases, the div macros trap division by zero. We're only picking between the conditional trap instructions ('teq' and similar) and the unconditional 'break' instruction (with a conditional branch over it), both of which are kinds of trap.

I'm not sure what to suggest for the spelling since most names fall into the same problem. Maybe 'use-tcc-in-div'?


http://reviews.llvm.org/D11675