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

[Diana Picus via Phabricator via llvm-commits]

rovka added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/InstructionSelector.cpp:46
 
+    // Predicate operands and optional defs don't need to be constrained.
+    MCOperandInfo OpInfo = I.getDesc().OpInfo[OpI];
----------------
ab wrote:
> rovka wrote:
> > rovka wrote:
> > > t.p.northover wrote:
> > > > rovka wrote:
> > > > > I'm not 100% sure that this is true in the general case. Thoughts?
> > > > Seems a bit iffy. What was triggering it in ARM?
> > > 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 :)
> > @tstellarAMD : I see that some R600 instructions have predicate operands, do you have any thoughts on how it would be best to handle them?
> 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).
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.


================
Comment at: lib/Target/ARM/ARMInstructionSelector.cpp:75-77
+    if (I.isCopy()) {
+      return selectCopy(I, TII, MRI, TRI, RBI);
+    }
----------------
ab wrote:
> One-line -> no braces (here and elsewhere)
Derp. Thanks.


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


================
Comment at: lib/Target/ARM/ARMRegisterBankInfo.cpp:111
+    ValueMapping *OperandsMapping = &ARM::ValueMappings[0];
+    return InstructionMapping{DefaultMappingID, Cost, OperandsMapping,
+                              NumOperands};
----------------
ab wrote:
> Nitpick:
> 
>   ..., /*Cost=*/1, ...
> 
> instead of a separate variable?
Makes sense.


================
Comment at: test/CodeGen/ARM/GlobalISel/arm-legalizer.mir:3
+--- |
+  define void @test_adds32() { ret void }
+...
----------------
ab wrote:
> Nitpick: I'd call this add_s32;  I initially parsed it as adds+32, not add+s32 ;)
Good point :D


https://reviews.llvm.org/D26677