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

[Daniel Sanders]

> + TargetSupported = ((Subtarget->getRelocationModel() == Reloc::PIC_) &&

>  + ((Subtarget->hasMips32r2() ||

>  Subtarget->hasMips32()) &&

>  + (Subtarget->isABI_O32())));

> 

> Ditto.


I'd like to get rid of TargetSupported and replace it with more precisely targetted checks. I don't like the way we can't even do addition of two i32's if we aren't generating PIC code. When it was last discussed, I relented and allowed it for now on the understanding that it will be removed in the future.

> + unsigned Offset = Addr.getOffset();

>  + ;

>  + MachineFrameInfo &MFI = *MF->getFrameInfo();

>  + ;

> 

> ?


I'm not sure how I missed those in the earlier reviews. There seem to be a few of them dotted around. I've noted the ones I've spotted (including the ones you pointed out). They should probably be fixed in a separate commit to avoid adding noise to this one.


================
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);
----------------
dsanders wrote:
> Nit: Capitalize first letter and delete blank comment line.
Done

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

================
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)
----------------
dsanders wrote:
> 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.
Done (comment was removed)

================
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;
----------------
dsanders wrote:
> Nit: Indentation
Done

================
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());
----------------
rkotler wrote:
> dsanders wrote:
> > 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
> I've made the change so that it returns an Address but personally I think that this makes it harder to read and I don't see why it's confusing to modify a reference variable in C++, that's what they are there for.
They have value syntax which makes them indistinguishable from by-value arguments. If you were already using the return value for something else then I wouldn't have mentioned it but on this occasion we are able to be explicit and explicit is better than implicit.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:673
@@ -616,3 +672,3 @@
     MachineFrameInfo &MFI = *MF->getFrameInfo();
     ;
     MachineMemOperand *MMO = MF->getMachineMemOperand(
----------------
Nit: Redundant semi-colon

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:1371
@@ -1314,1 +1370,3 @@
 
+void MipsFastISel::ensureOffset(Address &Addr, bool FitsOffset) {
+  if (FitsOffset)
----------------
rkotler wrote:
> dsanders wrote:
> > Nit: What are we ensuring about the offset? Shouldn't we use ensureOffsetWithinRange or something along those lines?
> I've changed the name to ensureOffsetFits.
> 
> If the offset does not fit, then the integer is materialized and the signness is taken care of there.
Thanks

================
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);
----------------
dsanders wrote:
> 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().
This one hasn't been done

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:665
@@ +664,3 @@
+    unsigned Offset = Addr.getOffset();
+    ;
+    MachineFrameInfo &MFI = *MF->getFrameInfo();
----------------
Nit: Redundant semi-colon

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:667
@@ +666,3 @@
+    MachineFrameInfo &MFI = *MF->getFrameInfo();
+    ;
+    MachineMemOperand *MMO = MF->getMachineMemOperand(
----------------
Nit: Redundant semi-colon

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:1361
@@ -1246,1 +1360,3 @@
 
+MipsFastISel::Address MipsFastISel::ensureOffsetFits(Address Addr,
+                                                     bool FitsOffset) {
----------------
Nit: Redundant namespace? MipsFastISel::Address -> Address.

================
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]+]]
----------------
dsanders wrote:
> Nit: indentation
This nit hasn't been fixed.

================
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
----------------
dsanders wrote:
> 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.
Done

http://reviews.llvm.org/D6426

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