Page MenuHomePhabricator

Beginning of alloca implementation for Mips fast-isel
ClosedPublic

Authored by rkotler on Nov 26 2014, 2:43 PM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
dsanders added inline comments.Dec 22 2014, 10:24 AM
lib/Target/Mips/MipsFastISel.cpp
169–170

Nit: Capitalize first letter and delete blank comment line.

173

Nit: Unnecessary blank line

dsanders added inline comments.Feb 12 2015, 8:42 AM
lib/Target/Mips/MipsFastISel.cpp
355–356

To answer my own question after re-reading the code: Because we're trying to break out of two for-loops and the break at the target is for the switch-statement.

rkotler updated this revision to Diff 19926.Feb 13 2015, 1:26 PM
rkotler edited edge metadata.

Patch has been updated to be consistent with current state of LLVM (this patch is old). This version should have no functional changes and has been checked against test-suite at O0/O2 and for mips32 R1/R2.

rkotler updated this revision to Diff 19940.Feb 13 2015, 3:14 PM

Run clang format on patch.

Hi Reed,

Some quick inline comments.

+ assert(isFIBase() && "Invalid base frame index access!");

formatting.

  • const MipsSubtarget *Subtarget; const TargetInstrInfo &TII; const TargetLowering &TLI;

+ const MipsSubtarget *Subtarget;

Unnecessary.

+ Subtarget(&TM.getSubtarget<MipsSubtarget>()) {

No.

+ TargetSupported = ((Subtarget->getRelocationModel() == Reloc::PIC_) &&
+ ((Subtarget->hasMips32r2() ||
Subtarget->hasMips32()) &&
+ (Subtarget->isABI_O32())));

Ditto.

+ This code is mostly cloned from AArch64 (which cloned it from earlier
+
ports)'

Unnecessary.

+ unsigned Offset = Addr.getOffset();
+ ;
+ MachineFrameInfo &MFI = *MF->getFrameInfo();
+ ;

?

-eric

Not sure I understand some of these comments.

Unnecessary = "You shouldn't need to make this change"
No = "You can't make this change"

-eric

Hi Eric,

I appreciate you taking the time to do the review but please make your
comments using phabricator using the phabricator online
interface so that you can put them right where the source goes.

When you make one block it's hard to track them and in this case when I
responded to the email, it threw
away my response to your comments from the phabricator.

Are your comments all formatting issues?

In general we are using clang-format -i for all new Mips code.

Are you looking at this patch in Phabricator so you can see how the code
really looks when the patch is applied?

Reed

Subtarget is initialized first and then TargetSupported.

It may have been a bad choice of name but TargetSupported is derived
from Subtarget.
That name has been in Mips fast-isel for a long time now. It's a local
variable in the mips fast-isel class.

Probably should have been called SubtargetSupported.

Unfortunately, my patches that I'm trying to finally commit now are
very old and they have been blocked
in the patch review queue for months now.

Some things are getting done during auto merge that may not be intended.

Since the original patches some other people have made some cosmetic
changes to mips fast-isel and some
other things have changed in llvm.

I can go back and look at the patch history for these lines but if you
have something specific you want you
can just say it.

I see that you and Chandler made some changes in this area.

I will go back and make sure that this change does not undo those.

I'm going to back to redo this patch sequence.

The auto merge of my old patch did not work and I need to go back and do
it all by hand
from the master branch so that you and Chandlers patch are not undone in
any way.

Thanks for noticing that.

Reed

Cool deal. Thanks!

-eric

rkotler updated this revision to Diff 19960.Feb 13 2015, 9:24 PM

This hopefully freshens up the patch to current tip of tree without undoing any of Chandler or Eric Cs changes.
I am rerunning test-suite and then will make Daniels requested changes and any of Erics which still remain
after this rebasing.

Will upload changed source reflecting the latest comments I posted.

lib/Target/Mips/MipsFastISel.cpp
651

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.

1354

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.

rkotler updated this revision to Diff 20133.EditedFeb 17 2015, 7:08 PM

Test suite passes on O0/O2 for Mips 32 r1/r2.

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

lib/Target/Mips/MipsFastISel.cpp
169–170

Done

173

Done

318–319

Done (comment was removed)

318–330

Done

651

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.

660

Nit: Redundant semi-colon

662

Nit: Redundant semi-colon

672–673

Nit: Redundant semi-colon

1354

Thanks

1354

Nit: Redundant namespace? MipsFastISel::Address -> Address.

1355–1356

This one hasn't been done

test/CodeGen/Mips/Fast-ISel/overflt.ll
19

This nit hasn't been fixed.

45–46

Done

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.

lib/Target/Mips/MipsFastISel.cpp
660

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.

1354

Address is a local type of class MipsFastISel so it's not visible at this point.

test/CodeGen/Mips/Fast-ISel/overflt.ll
19

I think this was just because I did not run untabify on this.

45–46

That was a typo. It should have just been another use of Y_IDX

rkotler updated this revision to Diff 20216.Feb 18 2015, 1:16 PM

Incorporate latest comments.

New patch coming... something was wrong with this diff.

rkotler updated this revision to Diff 20217.Feb 18 2015, 1:46 PM

Missed a pair of superfluous semicolons.

Some inline comments.

lib/Target/Mips/MipsFastISel.cpp
1

Could you fix this comment (in a separate commit) while you're at it?

9

You don't need both includes.

319–330

I might be missing something but Ty looks unused except for passing it down to other invocations of computeAddress?

1354

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.

test/CodeGen/Mips/Fast-ISel/overflt.ll
11

Is this straight from C code or did you construct this up?

rkotler updated this revision to Diff 20229.Feb 18 2015, 3:40 PM

Add one more requested change. Will rerun test-suite at O0/O2 and mips32 r1/r2 on this change.

Add one more requested change. Will rerun test-suite at O0/O2 and mips32 r1/r2 on this change.

This didn't fix anything I brought out in my inline comments from the previous revision. Please take a look at those.

Thanks.

-eric

Sorry. I will make your changes.

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

rkotler updated this revision to Diff 20233.Feb 18 2015, 4:25 PM

Incorporate Eric Christopher's changes.

You missed a few comments. I've redone them inline and added more.

Thanks!

-eric

lib/Target/Mips/MipsFastISel.cpp
2

Go ahead and fix this up in a separate commit. Pre-approved.

319–330

Ty appears still unused?

652

I think the frequent offset checks are going to be inherently buggy. How about doing something ala ARMComputeAddress (or the equivalent in aarch64)?

test/CodeGen/Mips/Fast-ISel/overflt.ll
7

Is this from C code?

rkotler updated this revision to Diff 20250.Feb 18 2015, 6:26 PM

Eliminate unused parameter in ComputeAddress per Eric Christophers suggestion.

I will rerun test-suite after.

Please respond to and/or handle the rest of my comments.

Thanks.

-eric

rkotler added inline comments.Feb 18 2015, 6:56 PM
lib/Target/Mips/MipsFastISel.cpp
1

Okay. I've copied the header for AArch64, making appropriate changes for Mips.

9

fixed

319–330

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.

319–330

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.

652

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?

1354

In my latest change I removed the boolean. Daniel suggested the same thing. I also renamed it to materializeOffset.

test/CodeGen/Mips/Fast-ISel/overflt.ll
7

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.

11

I often start with C code because my make check tests are from executable tests that have been checked.

Sorry.

Had responded but did not hit the "submit" button.

Replies inline.

lib/Target/Mips/MipsFastISel.cpp
1

As I mentioned, please do this in a separate commit. It's also preapproved so just commit. Thanks.

652

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.

test/CodeGen/Mips/Fast-ISel/overflt.ll
7

It works well then if you put the C code that you used in the file so that it can just be regenerated if something changes as a comment.

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.

lib/Target/Mips/MipsFastISel.cpp
652

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.

660

Done

662

Done

1354

Ok, presumably the argument doesn't need it since it gets the namespace from the function. Thanks for checking.

rkotler updated this revision to Diff 20355.Feb 19 2015, 4:31 PM

Am in the processing of verifying this patch with test-suite at O0/O2 for mips 23 r1/r2

test-suite passes on latest push on O0/O2 and mips32 R1/R2

dsanders accepted this revision.Feb 23 2015, 2:59 AM
dsanders edited edge metadata.

LGTM

lib/Target/Mips/MipsFastISel.cpp
1–3

Phabricator won't let me delete this inline comment for some reason but it will let me edit it.

I was going to ask why this change was reverted but I've gone back through the comments and answered my own question (it's being committed separately)

1354

I'm happy with this solution to the "it's not obvious the argument is modified" issue since the function name now implies the modification.

test/CodeGen/Mips/Fast-ISel/overflt.ll
56–61

Nit: The blank lines should probably be ';' since this is one big block comment

This revision is now accepted and ready to land.Feb 23 2015, 2:59 AM
rkotler closed this revision.Feb 23 2015, 6:38 PM