Page MenuHomePhabricator

[ARM] GlobalISel: Select add i32, i32
ClosedPublic

Authored by rovka on Nov 15 2016, 8:47 AM.

Details

Summary

Add the minimal support necessary to select a function that returns the sum of
two i32 values.

This includes some very naive and hardcoded argument/return lowering (to be
generalized in the future), as well as the handling of copy and add instructions
throughout the GlobalISel pipeline.

Diff Detail

Repository
rL LLVM

Event Timeline

rovka updated this revision to Diff 78011.Nov 15 2016, 8:47 AM
rovka retitled this revision from to [ARM] GlobalISel: Select add i32, i32.
rovka updated this object.
rovka added reviewers: qcolombet, t.p.northover.
rovka added subscribers: llvm-commits, rengolin, zvi.
rovka added inline comments.Nov 15 2016, 8:48 AM
lib/CodeGen/GlobalISel/InstructionSelector.cpp
46 ↗(On Diff #78011)

I'm not 100% sure that this is true in the general case. Thoughts?

t.p.northover added inline comments.Nov 17 2016, 12:57 PM
lib/CodeGen/GlobalISel/InstructionSelector.cpp
46 ↗(On Diff #78011)

Seems a bit iffy. What was triggering it in ARM?

lib/Target/ARM/ARMRegisterBankInfo.cpp
115 ↗(On Diff #78011)

You'll probably want this to be "return false" if you intend to use the SDAG fallback in your testing (and similarly for some other assertions).

Adding Ahmed as reviewer.

rovka added inline comments.Nov 18 2016, 4:33 AM
lib/CodeGen/GlobalISel/InstructionSelector.cpp
46 ↗(On Diff #78011)

For ARM, the predicate register (as added by AddDefaultPred) doesn't have a register class, and the CC register (which is an optional def) isn't found in MRI's VRegInfo (so we get an assert in MRI::getRegClassOrRegBank).

It's very possible that I'm missing something, but AFAICT the other isel frameworks don't call constrainRegClass on the predicate and optional defs either (at least for ARM). I don't know what is better - rolling a custom version of this function in the ARM backend, or skipping predicates and optional defs here and letting each target handle them. I'm open to other solutions too if you can suggest any :)

lib/Target/ARM/ARMRegisterBankInfo.cpp
115 ↗(On Diff #78011)

Right, I should probably start doing that. Thanks.

rovka updated this revision to Diff 79065.Nov 23 2016, 6:28 AM
rovka edited edge metadata.
rovka added a subscriber: tstellarAMD.

Rebase + some microscopic cleanups.

rovka added inline comments.Nov 23 2016, 6:32 AM
lib/CodeGen/GlobalISel/InstructionSelector.cpp
46 ↗(On Diff #78011)

@tstellarAMD : I see that some R600 instructions have predicate operands, do you have any thoughts on how it would be best to handle them?

rovka updated this revision to Diff 80282.Dec 5 2016, 9:54 AM

Make use of ValueHandler for lowering function arguments / returns.

ab requested changes to this revision.Dec 9 2016, 1:50 PM
ab edited edge metadata.

This seems mostly fine; first round of nitpicks ;)

Apologies for the long break.

lib/CodeGen/GlobalISel/InstructionSelector.cpp
46 ↗(On Diff #78011)

Shouldn't this simply be:

if (MO.getReg() == 0)
  continue;

?

That seems like the relevant characteristic here: both the predicate and CC optional def are either <x>PSR (a physreg, ignored earlier), or 0 (no-reg).

lib/Target/ARM/ARMInstructionSelector.cpp
75–77 ↗(On Diff #80282)

One-line -> no braces (here and elsewhere)

lib/Target/ARM/ARMRegisterBankInfo.cpp
56 ↗(On Diff #80282)

How about "GPRB"?

111 ↗(On Diff #80282)

Nitpick:

..., /*Cost=*/1, ...

instead of a separate variable?

test/CodeGen/ARM/GlobalISel/arm-legalizer.mir
3 ↗(On Diff #80282)

Nitpick: I'd call this add_s32; I initially parsed it as adds+32, not add+s32 ;)

This revision now requires changes to proceed.Dec 9 2016, 1:50 PM
rovka added inline comments.Dec 12 2016, 5:24 AM
lib/CodeGen/GlobalISel/InstructionSelector.cpp
46 ↗(On Diff #78011)

Ok, that works too. I was trying to be specific because I'm not very familiar with all the subtleties here, e.g. if there are other cases when a MO.getReg() could be 0 and we wouldn't want to skip it.

lib/Target/ARM/ARMInstructionSelector.cpp
75–77 ↗(On Diff #80282)

Derp. Thanks.

lib/Target/ARM/ARMRegisterBankInfo.cpp
56 ↗(On Diff #80282)

Sure, why not :)

Speaking of this, for AArch64 "GPR" is a reg bank, and for ARM it's a reg class. Would it be a good idea to have a target-independent convention for naming register banks? It could be a prefix / suffix added by TableGen to the name of the reg bank. I think it would make it easier to spot if a MIR file has been reg-bank-selected or not (without having to look for the regbankselected attribute).

111 ↗(On Diff #80282)

Makes sense.

test/CodeGen/ARM/GlobalISel/arm-legalizer.mir
3 ↗(On Diff #80282)

Good point :D

rovka updated this revision to Diff 81071.Dec 12 2016, 5:26 AM
rovka edited edge metadata.

Update based on Ahmed's feedback. Thanks!

ab accepted this revision.Dec 15 2016, 9:38 AM
ab edited edge metadata.

Looks good on my side, modulo a couple extra nitpicks (please do check the constraining in the test though).

lib/Target/ARM/ARMCallLowering.cpp
81–82 ↗(On Diff #81071)

Is this so that you can early-return in the simple case? FWIW I found it non-obvious; how about extracting the assignment code into some lowerReturnVal or something?

lib/Target/ARM/ARMISelLowering.h
493–494 ↗(On Diff #81071)

Not a big fan of adding these in this patch; maybe commit this separately (and use it in ISelLowering.cpp) ?

lib/Target/ARM/ARMRegisterBankInfo.cpp
56 ↗(On Diff #80282)

Speaking of this, for AArch64 "GPR" is a reg bank, and for ARM it's a reg class. Would it be a good idea to have a target-independent convention for naming register banks? It could be a prefix / suffix added by TableGen to the name of the reg bank.

It might; any ideas? We could also simply call the bank "GPR" on ARM. I don't think there's ever any ambiguity between bank name and class name (in MIR/C++, there might be ambiguity for humans). In fact, CCR is both the bank and the class on AArch64, but it's not really used, so problems might be hiding.

I think it would make it easier to spot if a MIR file has been reg-bank-selected or not (without having to look for the regbankselected attribute).

Eh, look for "class: _" ? ;)

test/CodeGen/ARM/GlobalISel/arm-instruction-select.mir
12–15 ↗(On Diff #81071)

Check the registers too?

This revision is now accepted and ready to land.Dec 15 2016, 9:38 AM
rovka added a comment.Dec 16 2016, 2:11 AM

Thanks for reviewing! I'll commit it with the requested changes.

Do you think you could have a look at the other patches in the series before going on vacation? https://reviews.llvm.org/D27803 + its dependencies.

Cheers,
Diana

lib/Target/ARM/ARMCallLowering.cpp
81–82 ↗(On Diff #81071)

Ok, sorry about the confusion.

lib/Target/ARM/ARMISelLowering.h
493–494 ↗(On Diff #81071)

Will do.

lib/Target/ARM/ARMRegisterBankInfo.cpp
56 ↗(On Diff #80282)

Actually there is some ambiguity. Initially I named the bank GPR and got some really funny errors when trying to read in the output of a regbankselect run. It turns out MIRParserImpl::parseRegisterInfo calls getRegClass and only if that doesn't return anything it calls getRegBank (and then we get "unexpected size on non-generic virtual register" later on). It actually took me a while to figure out what the problem was, but then I just assumed it went without saying that reg classes and reg banks should have different names. We should probably add some asserts somewhere if it's not obvious to you either that the names should be different. I thought it was just me :)

test/CodeGen/ARM/GlobalISel/arm-instruction-select.mir
12–15 ↗(On Diff #81071)

Good point.

This revision was automatically updated to reflect the committed changes.