Page MenuHomePhabricator

[globalisel] Select register bank for DBG_VALUE
ClosedPublic

Authored by LuoYuanke on Jul 2 2022, 2:51 AM.

Details

Summary

The register operand of DBG_VALUE is not selected to a proper register
bank in both AArch64 and X86. This would cause getRegClass crash after
global ISel. After discussion, we think the MIR should assume all
vritual register should be set proper register class after global ISel,
so this patch is to fix the gap of DBG_VALUE for AArch64 and X86.

Diff Detail

Event Timeline

LuoYuanke created this revision.Jul 2 2022, 2:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2022, 2:51 AM
LuoYuanke requested review of this revision.Jul 2 2022, 2:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2022, 2:51 AM

This patch is to fix the global ISel issue encountered in D126771 and http://45.33.8.238/macm1/37938/step_11.txt.

LuoYuanke added a reviewer: ab.Jul 4 2022, 5:48 PM
arsenm added inline comments.Jul 5 2022, 8:06 AM
llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
611–613

This should get a mir test that just runs regbankselect

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
910–911

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

912

This should guard the call, not be in it

920

While getType does check for physical registers, I think it shouldn't. Can you change !Reg to Reg.isPhysical()?

928–929

I don't understand why you are looping over to precheck all operands. You can process each register individually

937–952

This looks unnecessarily complicated, but I do think we have some API issues here

LuoYuanke added inline comments.Jul 5 2022, 6:18 PM
llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
611–613

We have test/CodeGen/AArch64/GlobalISel/arm64-regbankselect.mir to cover this scenario.

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
920

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().

928–929

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.

LuoYuanke added inline comments.Jul 5 2022, 6:49 PM
llvm/lib/Target/X86/X86RegisterBankInfo.cpp
117

This scenario can be covered by test/CodeGen/X86/GlobalISel/x86-calllowering-dbg-trunc.ll.

133

This scenario can be covered by test/CodeGen/X86/GlobalISel/x86-calllowering-dbg-trunc.ll.

LuoYuanke added inline comments.Jul 5 2022, 8:14 PM
llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
736

There are 3 cases that I encounter when debuging test case failure.

  1. 0 register.
  2. physical register.
  3. virtual register that has been assigned to a register class.

I changed the code to check Reg.isPhysical(), but I got a failure for the case 3. So I have to check if Ty is valid here.

DBG_VALUE %0:gpr32, $noreg, !"6", !DIExpression(), debug-location !35; test/CodeGen/AArch64/GlobalISel/arm64-regbankselect.mir:6:1 line no:6

LuoYuanke updated this revision to Diff 442436.Jul 5 2022, 8:54 PM

Address Arsenault's comments.

arsenm added inline comments.Jul 6 2022, 6:03 AM
llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
734

Typo virutal

738

I wouldn't assume if one operand doesn't need assignment, then none do. Treat each operand individually

llvm/test/CodeGen/AArch64/GlobalISel/select-hint.mir
17–21

These baseline check updates should be pre-committed

LuoYuanke added inline comments.Jul 6 2022, 4:26 PM
llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
738

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?

LuoYuanke updated this revision to Diff 442756.Jul 6 2022, 7:22 PM

Address Arsenault's comments.

LuoYuanke updated this revision to Diff 442775.Jul 6 2022, 10:06 PM

Add TODO for debug instructions and rebase.

@arsenm, is there anything more I need to address?

arsenm added inline comments.Jul 11 2022, 12:30 PM
llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp
738

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

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
928–929

Same, better to have simpler code that handles each register individually

LuoYuanke updated this revision to Diff 445288.Sat, Jul 16, 9:20 PM

Address Arsenault's comments.

LuoYuanke marked 2 inline comments as done.Wed, Jul 20, 8:17 AM

@arsenm, any other comments?

arsenm accepted this revision.Mon, Aug 8, 11:23 AM

LGTM, although this leaves AMDGPU and all other targets broken. I'm going to file a GitHub issue for this

llvm/lib/CodeGen/RegisterBankInfo.cpp
452–454

I almost think debug instrs shouldn't go through applyDefaultMapping but maybe it is easier this way

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
910

This should really go to generic code, but I won't insist on you doing that in this patch. We still need a better generic API for constraining. Can you add a fix to move this?

llvm/lib/Target/X86/X86InstructionSelector.cpp
235–237

Same, should get a fixme

This revision is now accepted and ready to land.Mon, Aug 8, 11:23 AM
LuoYuanke updated this revision to Diff 451036.Mon, Aug 8, 8:20 PM

Address Arsenault's comments. Thank you for review.

LuoYuanke marked 2 inline comments as done.Mon, Aug 8, 8:21 PM
This revision was landed with ongoing or failed builds.Mon, Aug 8, 10:14 PM
This revision was automatically updated to reflect the committed changes.