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

reed kotler rkotler at mips.com
Wed Feb 18 11:07:51 PST 2015

Getting rid of TargetSupported is possible but we need to add more build bots locally at Imagination to test for non pic and it's currently not in scope for my work now but management decided earlier that you can increase the scope but I just wanted to make it clear that this is an increase of scope of the project to non pic.

At this point, to expand this to non pic is not a big deal.

Comment at: lib/Target/Mips/MipsFastISel.cpp:665
@@ +664,3 @@
+    unsigned Offset = Addr.getOffset();
+    ;
+    MachineFrameInfo &MFI = *MF->getFrameInfo();
dsanders wrote:
> Nit: Redundant semi-colon
These are from cutting and pasting code from other ports and deleting a line but not the semicolon but sometimes they are hard to see until I run clang-format and they end up on their own line.

Everything compiles and all tests run so they are easy to miss because there is no functional difference whether they are there or not.

Comment at: lib/Target/Mips/MipsFastISel.cpp:1361
@@ -1246,1 +1360,3 @@
+MipsFastISel::Address MipsFastISel::ensureOffsetFits(Address Addr,
+                                                     bool FitsOffset) {
dsanders wrote:
> Nit: Redundant namespace? MipsFastISel::Address -> Address.
Address is a local type of class MipsFastISel so it's not visible at this point.

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
I think this was just because I did not run untabify on this.

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.
That was a typo. It should have just been another use of Y_IDX



More information about the llvm-commits mailing list