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

[Diana Picus 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];
----------------
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 :)


================
Comment at: lib/Target/ARM/ARMRegisterBankInfo.cpp:115
+
+  llvm_unreachable("Unsupported opcode");
+}
----------------
t.p.northover wrote:
> 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).
Right, I should probably start doing that. Thanks.


https://reviews.llvm.org/D26677