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

[reed kotler]

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:332
@@ -305,9 +331,3 @@
 
-bool MipsFastISel::computeAddress(const Value *Obj, Address &Addr) {
-  // This construct looks a big awkward but it is how other ports handle this
-  // and as this function is more fully completed, these cases which
-  // return false will have additional code in them.
-  //
-  if (isa<Instruction>(Obj))
-    return false;
-  else if (isa<ConstantExpr>(Obj))
+bool MipsFastISel::computeAddress(const Value *Obj, Address &Addr, Type *Ty) {
+
----------------
echristo wrote:
> Ty appears still unused?
Good catch. I just copied that code from AArch64 which also does not seem to use it.
It compiles fine without that. Will run test-suite again.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:667
@@ +666,3 @@
+  if (Addr.isRegBase()) {
+    if (!isInt<16>(Addr.getOffset()))
+      Addr = materializeOffset(Addr);
----------------
echristo wrote:
> I think the frequent offset checks are going to be inherently buggy. How about doing something ala ARMComputeAddress (or the equivalent in aarch64)?
I will investigate this idea for a future patch but I don't want to do more here. Good point though. This kind of code occurs all over llvm so it's not out of style. I have 10 patches in the queue for mips fast-isel and I need to get caught up.

Which code were you thinking of in ARMFastIsel?

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:1
@@ -1,2 +1,2 @@
 //===-- MipsastISel.cpp - Mips FastISel implementation
 //---------------------===//
----------------
echristo wrote:
> Could you fix this comment (in a separate commit) while you're at it?
Okay. I've copied the header for AArch64, making appropriate changes for Mips.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:9
@@ -7,2 +8,3 @@
 #include "MipsRegisterInfo.h"
+#include "MipsSEInstrInfo.h"
 #include "MipsSubtarget.h"
----------------
echristo wrote:
> You don't need both includes.
fixed

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:322
@@ -305,9 +321,3 @@
 
-bool MipsFastISel::computeAddress(const Value *Obj, Address &Addr) {
-  // This construct looks a big awkward but it is how other ports handle this
-  // and as this function is more fully completed, these cases which
-  // return false will have additional code in them.
-  //
-  if (isa<Instruction>(Obj))
-    return false;
-  else if (isa<ConstantExpr>(Obj))
+bool MipsFastISel::computeAddress(const Value *Obj, Address &Addr, Type *Ty) {
+
----------------
echristo wrote:
> I might be missing something but Ty looks unused except for passing it down to other invocations of computeAddress?
This is copied from the other ports. I'm trying to not make changes from the other ports except where necessary. Probably 75% of all the ports code should really be common code and at some time in the future I'm hoping that someone will refactor this.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:1358
@@ -1245,1 +1357,3 @@
 
+MipsFastISel::Address MipsFastISel::ensureOffsetFits(Address Addr,
+                                                     bool FitsOffset) {
----------------
echristo wrote:
> This routine is just weirdly named/parametered. I'd probably just remove the boolean parameter and do the check inside. It'll make much more sense from the outside too.
In my latest change I removed the boolean. Daniel suggested the same thing. I also renamed it to materializeOffset.

================
Comment at: test/CodeGen/Mips/Fast-ISel/overflt.ll:6
@@ +5,3 @@
+
+ at x = common global [128000 x float] zeroinitializer, align 4
+ at y = global float* getelementptr inbounds ([128000 x float]* @x, i32 0, i32 0), align 4
----------------
echristo wrote:
> Is this from C code?
Originally. Almost every make check test I have written was derived from a standalone executable test. Many, but not all, were originally C code. This one seems to be.

I make sure the executable test works and then usually lop off "main" and compile with -emit-llvm and turn it into an LLVM test.





================
Comment at: test/CodeGen/Mips/Fast-ISel/overflt.ll:10
@@ +9,3 @@
+ at .str = private unnamed_addr constant [5 x i8] c"%f \0A\00", align 1
+
+; Function Attrs: nounwind
----------------
echristo wrote:
> Is this straight from C code or did you construct this up?
I often start with C code because my make check tests are from executable tests that have been checked.

http://reviews.llvm.org/D6426

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