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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp:734
+          if (!Ty.isValid()) {
+            Ignore = true;
+            break;
----------------
LuoYuanke wrote:
> arsenm wrote:
> > I wouldn't assume if one operand doesn't need assignment, then none do. Treat each operand individually
> Yes, I agree, but this patch just to be conservative to constrain the logic in handling debug instruction, so that it can avoid causing much regression.
> I think inline assembly also has such issue, because it may be mixed with physical register and virtual register.
> How about adding a TODO here, so that we can improve it in another patch that handle the physical register or register that is assigned register class in underlying code?
I don't see this as conservative, just weirdly inconsistent. It would simplify the code and be more correct to just handle all virtual register operands


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp:928-929
+    }
+    if (Ignore)
+      return true;
+    for (MachineOperand &MO : I.operands()) {
----------------
LuoYuanke wrote:
> 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.
Same, better to have simpler code that handles each register individually


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129037