[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:606-608
+      // Don't insert additional instruction for debug instruction.
+      if (MI.isDebugInstr())
+        break;
----------------
This should get a mir test that just runs regbankselect


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp:910-911
 
+static bool selectDebugInstr(MachineInstr &I, MachineRegisterInfo &MRI,
+                             const RegisterBankInfo &RBI) {
+  if (I.isDebugInstr()) {
----------------
I would expect this to be handled by generic code. However, last I looked at handling selection of another pseudo generically, I think we were missing API to allow generic code to constrain registers


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp:912
+                             const RegisterBankInfo &RBI) {
+  if (I.isDebugInstr()) {
+    bool Ignore = false;
----------------
This should guard the call, not be in it


================
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
----------------
While getType does check for physical registers, I think it shouldn't. Can you change !Reg to Reg.isPhysical()?


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp:928-929
+    }
+    if (Ignore)
+      return true;
+    for (MachineOperand &MO : I.operands()) {
----------------
I don't understand why you are looping over to precheck all operands. You can process each register individually


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp:937-952
+      const RegClassOrRegBank &RegClassOrBank = MRI.getRegClassOrRegBank(Reg);
+      const TargetRegisterClass *RC =
+          RegClassOrBank.dyn_cast<const TargetRegisterClass *>();
+      if (!RC) {
+        const RegisterBank &RB = *RegClassOrBank.get<const RegisterBank *>();
+        RC = getRegClassForTypeOnBank(Ty, RB);
+        if (!RC) {
----------------
This looks unnecessarily complicated, but I do think we have some API issues here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129037