Closes #64452
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/include/llvm/CodeGen/MachineRegisterInfo.h | ||
---|---|---|
675 | Is GlobalISel also a instruction selection process? | |
677 | My understanding the or is correct, it looks to me like if (!GlobalISel || AfterISel) | |
llvm/test/CodeGen/X86/AMX/amx-fastpreconfig-gisel.mir | ||
4 ↗ | (On Diff #548421) | How can I check it's GlobalIsel MIR? I didn't find anything special in the test. |
llvm/include/llvm/CodeGen/MachineRegisterInfo.h | ||
---|---|---|
677 | Oh, I think and makes sense here. Sorry.. |
llvm/include/llvm/CodeGen/MachineRegisterInfo.h | ||
---|---|---|
675 | I got your point. I think it's better to list as 3 conditions: 2. MIR selected using GlobalISel. 3. The machine function has .. |
llvm/include/llvm/CodeGen/MachineRegisterInfo.h | ||
---|---|---|
675 | GlobalISel is an instruction selection process. | |
677 | The sentence was correct :).
| |
llvm/test/CodeGen/X86/AMX/amx-fastpreconfig-gisel.mir | ||
2 ↗ | (On Diff #548421) |
Could you do a print-after-all with your failure (starting back at LLVM IR level) and see where the regclass is not properly set? |
llvm/include/llvm/CodeGen/MachineRegisterInfo.h | ||
---|---|---|
677 | Yeah, but the second bullet is not correct now. It is not safe to use getRegClass after GlobalISel because after InstructionSelection pass we may have registers without a class. That's why I changed that we shouldn't select using GlobalISel at all and we shouldn't be in a selection process (by SelectionDAG or FastISel). Actually I'm not sure whether it is designed so or not. | |
llvm/test/CodeGen/X86/AMX/amx-fastpreconfig-gisel.mir | ||
2 ↗ | (On Diff #548421) | We don't need to pass -global-isel=1 because this MIR is already obtained from GlobalISel and llc just runs the problematic pass. On the previous iteration I put a test from .ll but I think it is not convenient to keep it actual. After specific changes in GlobalISel we need to update it and in the worst case we have to find another test when a register doesn't have an assigned class. MIR test solves this problem. Here is print-after-all output *** IR Dump After Module Verifier (verify) *** define i64 @f(i64 %0, i64 %1) { entry: %2 = lshr i64 %0, %1 %3 = add i64 %2, 123456789 ret i64 %3 } # *** IR Dump After IRTranslator (irtranslator) ***: # Machine code for function f: IsSSA, TracksLiveness Function Live Ins: $rdi, $rsi bb.1.entry: liveins: $rdi, $rsi %0:_(s64) = COPY $rdi %1:_(s64) = COPY $rsi %3:_(s64) = G_CONSTANT i64 123456789 %2:_(s64) = G_LSHR %0:_, %1:_(s64) %4:_(s64) = G_ADD %2:_, %3:_ $rax = COPY %4:_(s64) RET 0, implicit $rax # End machine code for function f. # *** IR Dump After Legalizer (legalizer) ***: # Machine code for function f: IsSSA, TracksLiveness, Legalized Function Live Ins: $rdi, $rsi bb.1.entry: liveins: $rdi, $rsi %0:_(s64) = COPY $rdi %1:_(s64) = COPY $rsi %3:_(s64) = G_CONSTANT i64 123456789 %5:_(s8) = G_TRUNC %1:_(s64) %2:_(s64) = G_LSHR %0:_, %5:_(s8) %4:_(s64) = G_ADD %2:_, %3:_ $rax = COPY %4:_(s64) RET 0, implicit $rax # End machine code for function f. # *** IR Dump After RegBankSelect (regbankselect) ***: # Machine code for function f: IsSSA, TracksLiveness, Legalized, RegBankSelected Function Live Ins: $rdi, $rsi bb.1.entry: liveins: $rdi, $rsi %0:gpr(s64) = COPY $rdi %1:gpr(s64) = COPY $rsi %3:gpr(s64) = G_CONSTANT i64 123456789 %5:gpr(s8) = G_TRUNC %1:gpr(s64) %2:gpr(s64) = G_LSHR %0:gpr, %5:gpr(s8) %4:gpr(s64) = G_ADD %2:gpr, %3:gpr $rax = COPY %4:gpr(s64) RET 0, implicit $rax # End machine code for function f. # *** IR Dump After InstructionSelect (instruction-select) ***: # Machine code for function f: IsSSA, TracksLiveness, Legalized, RegBankSelected, Selected Function Live Ins: $rdi, $rsi bb.1.entry: liveins: $rdi, $rsi %0:gr64 = COPY $rdi %1:gr64_with_sub_8bit = COPY $rsi %5:gr8 = COPY %1.sub_8bit:gr64_with_sub_8bit $cl = COPY %5:gr8 %2:gr64 = SHR64rCL %0:gr64(tied-def 0), implicit-def $eflags, implicit $cl %4:gr64 = ADD64ri32 %2:gr64(tied-def 0), 123456789, implicit-def $eflags $rax = COPY %4:gr64 RET 0, implicit $rax # End machine code for function f. # *** IR Dump After Finalize ISel and expand pseudo-instructions (finalize-isel) ***: # Machine code for function f: IsSSA, TracksLiveness, Legalized, RegBankSelected, Selected Function Live Ins: $rdi, $rsi bb.1.entry: liveins: $rdi, $rsi %0:gr64 = COPY $rdi %1:gr64_with_sub_8bit = COPY $rsi %5:gr8 = COPY %1.sub_8bit:gr64_with_sub_8bit $cl = COPY %5:gr8 %2:gr64 = SHR64rCL %0:gr64(tied-def 0), implicit-def $eflags, implicit $cl %4:gr64 = ADD64ri32 %2:gr64(tied-def 0), 123456789, implicit-def $eflags $rax = COPY %4:gr64 RET 0, implicit $rax A virtual register %3 is absent after instruction-select and doesn't have an assigned register class, only a register bank. Selecting: %3:gpr(s64) = G_CONSTANT i64 123456789 Is dead; erasing. The test is obtained using -stop-after, so you can see that %3 really has only a register bank not a class. The same I see on AArch64. Either both targets do something wrong or the problem is in general GISel algorithm. But we do have null regclass after GISel. |
4 ↗ | (On Diff #548421) | Yes, I've put a note
- { id: 3, class: gpr, preferred-register: '' } |
llvm/test/CodeGen/X86/AMX/amx-fastpreconfig-gisel.mir | ||
---|---|---|
2 ↗ | (On Diff #548421) | Yes, it is pretty simple: https://github.com/llvm/llvm-project/commit/931904d7772b18210d7166ed657176ae91ad11d8
|
llvm/test/CodeGen/X86/AMX/amx-fastpreconfig-gisel.mir | ||
---|---|---|
4 ↗ | (On Diff #548421) | I've answered not existing question previously... Unfortunately there is no explicit sign about GlobalIsel, however there is implicit: legalized: true regBankSelected: true selected: true We have them all set to false in case of SelectionDAG. |
llvm/test/CodeGen/X86/AMX/amx-fastpreconfig-gisel.mir | ||
---|---|---|
2 ↗ | (On Diff #548421) | @qcolombet reminds me, X86FastPreTileConfig was intended to solve for FastISel issues. I don't think it can work with GISel. It simply checks OptLevel because we just needs to distinguish FastISel from SDISel in the past. The introduction of GISel makes it's invalid. I think we should introduce a method to check the MIR was generated by which one and run this pass only for FastISel. |
llvm/lib/Target/X86/X86FastPreTileConfig.cpp | ||
---|---|---|
672 ↗ | (On Diff #548421) | I think the proper fix here would be: if (!MRI->reg_empty(VirtReg) && // <-- only process alive registers MRI->getRegClass(VirtReg)->getID() == X86::TILERegClassID) { |
llvm/test/CodeGen/X86/AMX/amx-fastpreconfig-gisel.mir | ||
2 ↗ | (On Diff #548421) | Ok I understand now. It cannot happen with SDISel because the registers are instantiated when the actual instructions are produced. In any case, I believe the fix is wrong: we shouldn't process dead registers. All bets are off on these. E.g., after regalloc they won't have a physreg attached to them and that's ok. Here is what I would suggest:
|
LGTM.
llvm/lib/Target/X86/X86TargetMachine.cpp | ||
---|---|---|
538 | I'm not sure if this pass works for GISel or not, let see if there's problem in the future. |
Change the comment to say that only alive registers are guaranteed to have a regclass.
@qcolombet I've added a part about alive registers. It's unclear to me, should we drop a part about 2 conditions or keep it?
llvm/lib/Target/X86/X86FastPreTileConfig.cpp | ||
---|---|---|
672 ↗ | (On Diff #548421) | @qcolombet thank you! This fix is really better than just skipping registers. However since X86FastPreTileConfig is intended for FastISel only. I've decided to continue this way. |
llvm/lib/Target/X86/X86TargetMachine.cpp | ||
538 | Yes. I have the same thoughts. |
I've added a part about alive registers.
I didn't see that part. I've suggested one as a result (https://reviews.llvm.org/D157458?id=548421#inline-1527304 and https://reviews.llvm.org/D157458?id=548421#inline-1527304).
llvm/include/llvm/CodeGen/MachineRegisterInfo.h | ||
---|---|---|
675 | Instead of modifying #2, I would say: 2. [...], or 3. The virtual register is dead and didn't get through the selection process. | |
677 | Maybe we could simplify the sentence and only say, it is safe to use getRegClass when we are after isel and reg is alive (!MRI->reg_empty(VirtReg)). | |
llvm/lib/Target/X86/X86FastPreTileConfig.cpp | ||
672 ↗ | (On Diff #548421) | Should you assert that RC is not nullptr then? And add a message that this pass is intended to run after FastISel. E.g., assert(RC && "This shouldn't happen when registers are created through FastISel"); |
llvm/test/CodeGen/X86/AMX/amx-fastpreconfig-gisel.mir | ||
6 ↗ | (On Diff #548421) | I guess I'll phrase the test case differently: Make sure `fastpretileconfig` doesn't choke on virtual registers that didn't get through the instruction selection process. This may happen when a virtual register is created during the isel process and that code gets optimized away before it gets through the final instruction selection stage. In this particular test, this can be seen with reg id 3 having no register class (only a register bank). |
@qcolombet I've updated the comment. Do you think we still need to modify X86FastPreTileConfig? In the latest version we disable this pass for GlobalISel because it was intended for the code selected by FastISel. It's unclear whether we need it with GlobalISel in the future or not.
Do you think we still need to modify X86FastPreTileConfig?
My suggestion would be to use getRegClassOrNull and assert that RC is not nullptr and add a message in the assert that if that assert fails that means we're not running with FastISel and that RC should actually be checked.
Just to be on the safe side of not rediscovery this issue if that pass ends up being run with GISel.
llvm/include/llvm/CodeGen/MachineRegisterInfo.h | ||
---|---|---|
676 | Typo: asigned => assigned |
Post-selection code should not be tolerant of registers without a class. It is unequivocally a bug for a used register to pass selection without a class dead or not
The comment changes in MRI seem unrelated to the actual change, can you post those separately?
I've created an issue https://github.com/llvm/llvm-project/issues/66531. However it is not used, that's why it is deleted, but yes, it is still present.
Now when we consider it as a bug, do we still need a comment? Or put it until fix appears?
@arsenm The key thing is that the register is not used. It just exists in the MRI table at this point.
It is created somewhere in GISel, but then it is dead before we reach instruction selection, hence we don't get any regclass assigned.
Is GlobalISel also a instruction selection process?