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

[Ahmed Bougacha via Phabricator via llvm-commits]

ab accepted this revision.
ab added a comment.
This revision is now accepted and ready to land.

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



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


================
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;
+
----------------
Not a big fan of adding these in this patch;  maybe commit this separately (and use it in ISelLowering.cpp) ?


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


================
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 }
----------------
Check the registers too?


https://reviews.llvm.org/D26677