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

[Daniel Sanders]

I think we're pretty close now. To summarize, I think we just have the following todo:

- indentation nit on the 'lui' in the test case.
- adding the original C source for the test case.
- make materializeOffset() a little more ARMSimplifyAddress()-like by moving an if-statement inside it.

> 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.


I'm not saying we need to remove it now (or even soon). I'm just mentioning the background and the plan since I don't expect anyone will remember a discussion from June/July.

> Daniel sent me some private email and i was doing those. I did not see your comments yet.


To be clear, this email didn't request anything that wasn't already in the review. We were discussing the review comments that hadn't been handled in the most recent patch.


================
Comment at: lib/Target/Mips/MipsFastISel.cpp:667
@@ +666,3 @@
+  if (Addr.isRegBase()) {
+    if (!isInt<16>(Addr.getOffset()))
+      Addr = materializeOffset(Addr);
----------------
echristo wrote:
> rkotler wrote:
> > 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?
> We fix this kind of code as we see it and it's in review right now (also, since you're in here and handling all of this right now). The name of the function is ARMSimplifyAddress and it's immediately after ARMComputeAddress.
While discussing this with Reed, I've previously said that I'd be happy so long as the condition was either fully inside or fully outside the function rather than the mixture that was in the earlier code. However, I also said that fully inside would be better in the long run since we'll eventually end up there when we add support for various kinds of offsets (e.g. MSA's 10-bit scaled offsets). I've had a quick look at ARMSimplifyAddress and it's pretty much how I picture materializeOffset() ending up.

That said, given that Eric has brought this up too I think we might as well do it now. I believe all we need to do at this stage is move this if-statement inside the function to resolve Eric's comment.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:665
@@ +664,3 @@
+    unsigned Offset = Addr.getOffset();
+    ;
+    MachineFrameInfo &MFI = *MF->getFrameInfo();
----------------
rkotler wrote:
> 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.
> 
> 
Done

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

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:1361
@@ -1246,1 +1360,3 @@
 
+MipsFastISel::Address MipsFastISel::ensureOffsetFits(Address Addr,
+                                                     bool FitsOffset) {
----------------
rkotler wrote:
> dsanders wrote:
> > Nit: Redundant namespace? MipsFastISel::Address -> Address.
> Address is a local type of class MipsFastISel so it's not visible at this point.
Ok, presumably the argument doesn't need it since it gets the namespace from the function. Thanks for checking.

http://reviews.llvm.org/D6426

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