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

[Ahmed Bougacha via Phabricator via llvm-commits]

ab requested changes to this revision.
ab added a comment.
This revision now requires changes to proceed.

This seems mostly fine;  first round of nitpicks ;)

Apologies for the long break.



================
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];
----------------
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).


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


================
Comment at: lib/Target/ARM/ARMRegisterBankInfo.cpp:56
+  // Initialize the GPR bank.
+  createRegisterBank(ARM::GPRRegBankID, "GPRrb");
+
----------------
How about "GPRB"?


================
Comment at: lib/Target/ARM/ARMRegisterBankInfo.cpp:111
+    ValueMapping *OperandsMapping = &ARM::ValueMappings[0];
+    return InstructionMapping{DefaultMappingID, Cost, OperandsMapping,
+                              NumOperands};
----------------
Nitpick:

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

instead of a separate variable?


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


https://reviews.llvm.org/D26677