[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 accepted this revision.
dsanders added a comment.
This revision is now accepted and ready to land.

Ignoring the magic number issue (see below): LGTM with some formatting nits.

Regarding the magic number issue: I still don't have a good solution to eliminate these hard coded offsets so we should go ahead for now and eliminate them when we refactor the way expansions are handled (which we need to do so sort out other relocation-related issues).


================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2619
@@ +2618,3 @@
+      }
+    } else {
+      emitRR(DivOp, RsReg, RtReg, IDLoc, Instructions);
----------------
dsanders wrote:
> Style nit: Avoid else after return
I know why you've skipped this comment because we spoke face to face but you should also mention it on list.

The reason is that it isn't an else after return. Only two of the three paths in the if-statements true case end in a return.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2595
@@ +2594,3 @@
+
+  //FIXME: The values for these two BranchTarget variables may be different in
+	// micromips. These magic numbers need to be removed.
----------------
Nit: Space after //

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2601-2604
@@ +2600,6 @@
+  if (UseTraps) {
+    if (IsMips64)
+      BranchTarget = 12;
+    else
+      BranchTarget = 8;
+    emitRRI(Mips::TEQ, RtReg, ZeroReg, 0x7, IDLoc, Instructions);
----------------
Nit: Use ternary operator

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2607-2610
@@ +2606,6 @@
+  } else {
+    if (IsMips64)
+      BranchTarget = 20;
+    else
+      BranchTarget = 16;
+    BranchTargetNoTraps = 8;
----------------
Nit: Use ternary operator

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2612
@@ +2611,3 @@
+    BranchTargetNoTraps = 8;
+    emitRRI(Mips::BNE, RtReg, ZeroReg, BranchTargetNoTraps, IDLoc, Instructions); //branch to the li instruction
+  }
----------------
Nit: 80cols? and space after //.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2631
@@ +2630,3 @@
+  if (IsMips64) {
+    emitRRI(Mips::BNE, RtReg, ATReg, BranchTarget, IDLoc, Instructions); //branch to the mflo instruction
+    emitRRI(Mips::ADDiu, ATReg, ZeroReg, 1, IDLoc, Instructions);
----------------
Nit: 80cols? and space after //.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2635
@@ +2634,3 @@
+  } else {
+    emitRRI(Mips::BNE, RtReg, ATReg, BranchTarget, IDLoc, Instructions); //branch to the mflo instruction
+    emitRI(Mips::LUi, ATReg, (uint16_t)0x8000, IDLoc, Instructions);
----------------
Nit: 80cols? and space after //.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:2642
@@ +2641,3 @@
+  else {
+    emitRRI(Mips::BNE, RsReg, ATReg, BranchTargetNoTraps, IDLoc, Instructions); //branch to the mflo instruction
+    emitRRI(Mips::SLL, ZeroReg, ZeroReg, 0, IDLoc, Instructions);
----------------
Nit: 80cols? and space after //.

================
Comment at: lib/Target/Mips/Mips.td:166-168
@@ -165,2 +165,5 @@
                                 [FeatureMips64r2]>;
+def FeatureUseTCCInDIV : SubtargetFeature<"use-tcc-in-div",
+                               "UseTCCInDIV", "false",
+                               "Force the assembler to use trapping">;
 
----------------
Nit: First argument should be hanging too.

The same is true of FeatureCnMips above but that's not for this patch.

================
Comment at: lib/Target/Mips/MipsInstrInfo.td:1728
@@ +1727,3 @@
+def SDivMacro : MipsAsmPseudoInst<(outs), (ins GPR32Opnd:$rs, GPR32Opnd:$rt),
+                            "div\t$rs, $rt">, ISA_MIPS1_NOT_32R6_64R6;
+
----------------
Nit: Indentation.

Likewise for the other 3.


http://reviews.llvm.org/D11675