[PATCH] D26677: [ARM] GlobalISel: Select add i32, i32

[Diana Picus via Phabricator via llvm-commits]

rovka added a comment.

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



================
Comment at: lib/Target/ARM/ARMCallLowering.cpp:81-82
+
+  // Move the insertion point before the return
+  MIRBuilder.setInstr(*Ret);
+
----------------
ab wrote:
> 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?
Ok, sorry about the confusion.


================
Comment at: lib/Target/ARM/ARMISelLowering.h:493-494
 
+    CCAssignFn *CCAssignFnForCall(CallingConv::ID CC, bool isVarArg) const;
+    CCAssignFn *CCAssignFnForReturn(CallingConv::ID CC, bool isVarArg) const;
+
----------------
ab wrote:
> Not a big fan of adding these in this patch;  maybe commit this separately (and use it in ISelLowering.cpp) ?
Will do.


================
Comment at: lib/Target/ARM/ARMRegisterBankInfo.cpp:56
+  // Initialize the GPR bank.
+  createRegisterBank(ARM::GPRRegBankID, "GPRrb");
+
----------------
ab wrote:
> rovka wrote:
> > ab wrote:
> > > How about "GPRB"?
> > 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).
> > 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: _" ? ;)
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 :) 


================
Comment at: test/CodeGen/ARM/GlobalISel/arm-instruction-select.mir:12-15
+registers:
+  - { id: 0, class: gprb }
+  - { id: 1, class: gprb }
+  - { id: 2, class: gprb }
----------------
ab wrote:
> Check the registers too?
Good point.


https://reviews.llvm.org/D26677