[PATCH] D129037: [globalisel] Select register bank for DBG_VALUE

[LuoYuanke via Phabricator via llvm-commits]

LuoYuanke added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp:606-608
+      // Don't insert additional instruction for debug instruction.
+      if (MI.isDebugInstr())
+        break;
----------------
arsenm wrote:
> This should get a mir test that just runs regbankselect
We have `test/CodeGen/AArch64/GlobalISel/arm64-regbankselect.mir` to cover this scenario.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp:920
+        continue;
+      LLT Ty = MRI.getType(Reg);
+      // It may be physical register or non register. For those cases, just
----------------
arsenm wrote:
> While getType does check for physical registers, I think it shouldn't. Can you change !Reg to Reg.isPhysical()?
This follow the same check of `RegBankSelect.cpp:730`. The RegBankSelect accept 0 register, but not accept physical register. I can add another check for Reg.isPhysical().


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp:928-929
+    }
+    if (Ignore)
+      return true;
+    for (MachineOperand &MO : I.operands()) {
----------------
arsenm wrote:
> I don't understand why you are looping over to precheck all operands. You can process each register individually
The precheck is to make sure there is no physical register or invalid type for an operand. If there is one, then we bail out for the whole operands. This is to avoid such scenario we handled some virtual register before we encounter a physical register operand.
We either hanldle all operands or we abandon for the whole instruction.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129037/new/

https://reviews.llvm.org/D129037