[PATCH] Beginning of alloca implementation for Mips fast-isel

[Daniel Sanders]

Sorry it's taken me so long to get to your patches.


================
Comment at: lib/Target/Mips/MipsFastISel.cpp:170-171
@@ +169,4 @@
+private:
+  // if the offset does not fit then materialize it in a register first.
+  //
+  void ensureOffset(Address &Addr, bool FitsOffset);
----------------
Nit: Capitalize first letter and delete blank comment line.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:174
@@ -169,1 +173,3 @@
+
+
 public:
----------------
Nit: Unnecessary blank line

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:329
@@ -322,3 +328,3 @@
   //
   // This code is mostly cloned from AARch64 (which cloned it from earlier
   // ports)
----------------
Nit: I know this is from an earlier patch but the R in AARch64 should be lowercase and there's an unnecessary blank comment line on the line above.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:380-381
@@ +379,4 @@
+           }
+           // Unsupported
+           goto unsupported_gep;
+         }
----------------
I see that the AArch64 port did this too but why use goto when the target of the goto simply does 'break'?

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:390
@@ -346,4 +389,3 @@
         return true;
-      }
-      break;
-    }
+      // We failed, restore everything and try the other options.
+     Addr = SavedAddr;
----------------
Nit: Indentation

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:664
@@ -608,2 +663,3 @@
   if (Addr.isRegBase()) {
+    ensureOffset(Addr, isInt<16>(Addr.getOffset()));
     emitInstStore(Opc, SrcReg, Addr.getReg(), Addr.getOffset());
----------------
It's not very obvious that we're changing Addr here (and at the other points this is called). Returning the new reference would be clearer

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:1371
@@ -1314,1 +1370,3 @@
 
+void MipsFastISel::ensureOffset(Address &Addr, bool FitsOffset) {
+  if (FitsOffset)
----------------
Nit: What are we ensuring about the offset? Shouldn't we use ensureOffsetWithinRange or something along those lines?

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:1372-1373
@@ +1371,4 @@
+void MipsFastISel::ensureOffset(Address &Addr, bool FitsOffset) {
+  if (FitsOffset)
+    return;
+  unsigned TempReg = materialize32BitInt(Addr.getOffset(), &Mips::GPR32RegClass);
----------------
This if-statement is half-inside and half-outside this function which feels a bit odd. I think it would be best to pass in the size of the constant (and maybe the signedness) and have the if-statement fully inside ensureOffset().

================
Comment at: test/CodeGen/Mips/Fast-ISel/overflt.ll:18
@@ +17,3 @@
+  store float 5.500000e+00, float* %arrayidx, align 4
+; CHECK: 	lui	$[[REG_FPCONST_INT:[0-9]+]], 16560
+; CHECK:	mtc1	$[[REG_FPCONST_INT]], $f[[REG_FPCONST:[0-9]+]]
----------------
Nit: indentation

================
Comment at: test/CodeGen/Mips/Fast-ISel/overflt.ll:44-45
@@ +43,4 @@
+; CHECK-DAG:	addu	$[[REG_Y_IDX:[0-9]+]], $[[REG_IDX]], $[[REG_Y]]
+; CHECK-DAG:	lwc1	$f[[Y_IDX:[0-9]+]], 0($[[REG_Y_IDX]])
+; CHECK-DAG:	swc1	$f[[Y_IDX:[0-9]+]], 0($[[REG_RESULT]])
+; CHECK-LABEL:   .end  goo
----------------
You define Y_IDX twice here. Shouldn't the second one be a usage.
I'm also not sure about the name Y_IDX since it's not an index.

http://reviews.llvm.org/D6426

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/