Open DimitryAndric opened 8 months ago
Reduced MIR test:
# RUN: llc -mtriple=x86_64-unknown-unknown -run-pass=machine-scheduler -verify-misched -o - %s
---
name: vga_load_state
tracksRegLiveness: true
body: |
bb.0:
liveins: $rdi
%0:gr64 = COPY $rdi
undef %1.sub_32bit:gr64_nosp = MOV32r0 implicit-def dead $eflags
dead $eax = MOV32r0 implicit-def dead $eflags, implicit-def $rax
INLINEASM &"", 1 /* sideeffect attdialect */
dead $eax = MOV32r0 implicit-def dead $eflags
INLINEASM &"", 1 /* sideeffect attdialect */
dead $eax = MOV32r0 implicit-def dead $eflags
dead $eax = MOV32r0 implicit-def dead $eflags, implicit-def $rax
...
So maybe for the time being, we should revert "RegisterCoalescer: Add implicit-def of super register when coalescing SUBREG_TO_REG" yet again? Or is that now too much hassle? It seems to have gone in and out several times now... :)
(Still asserts with llvmorg-18-init-15938-g c1764a7842ac)
I'm also seeing a similar issue of this and it fails similarly when cross-compiling for windows assert for release/15.x and release/16.x. Reverting 0e46b49de433 seems to solve the issue for me as well. Thanks for the revert!
I haven't yet reverted it on main, just on my FreeBSD branch. If Matt currently doesn't have time to fix the underlying problem, I will revert it later today.
@arsenm Are there any updates on this? If this is taking a while to fix, could we please revert the patch so that downstream builds could continue to build?
@arsenm Are there any updates on this? If this is taking a while to fix, could we please revert the patch so that downstream builds could continue to build?
I've been debugging it for a few days; it's a pre-existing RegisterPressure/Scheduler bug but I'm not really getting anywhere with it. The symptoms disappear with -precompute-phys-liveness. The pathway that doesn't use accurate liveness information seems to not work correctly with multiple defs of overlapping registers
This was reverted in c4146121e940b6b853148c780568dee38b97382f. Another reproducer was posted at https://github.com/llvm/llvm-project/commit/0e46b49de43349f8cbb2a7d4c6badef6d16e31ae
More reduced test case:
# RUN: llc -mtriple=x86_64-unknown-unknown -run-pass=machine-scheduler -misched-bottomup -verify-misched -o - %s
# xUN: llc -mtriple=x86_64-unknown-unknown -run-pass=machine-scheduler -misched-topdown -verify-misched -o - %s
# xUN: llc -mtriple=x86_64-unknown-unknown -run-pass=machine-scheduler -misched-topdown -o - %s
---
name: vga_load_state
tracksRegLiveness: true
body: |
bb.0:
liveins: $rdi
INLINEASM &"", 1 /* sideeffect attdialect */
INLINEASM &"", 1 /* sideeffect attdialect */
INLINEASM &"", 1 /* sideeffect attdialect */
INLINEASM &"", 1 /* sideeffect attdialect */
$ebx = MOV32r0 implicit-def $eflags
dead $ecx = MOV32r0 implicit-def $eflags
$eax = MOV32r0 implicit-def $eflags
$eax = MOV32r0 implicit-def $eflags
...
Adding a real use of $eax makes this no longer sensitive to having cached liveness info
---
name: dead_register_def_pressure_tracking_underflow_assert
tracksRegLiveness: true
body: |
bb.0:
INLINEASM &"", 1 /* sideeffect attdialect */
INLINEASM &"", 1 /* sideeffect attdialect */
INLINEASM &"", 1 /* sideeffect attdialect */
INLINEASM &"", 1 /* sideeffect attdialect */
$ebx = MOV32r0 implicit-def $eflags
dead $ecx = MOV32r0 implicit-def $eflags
$eax = MOV32r1 implicit-def $eflags
$eax = MOV32r0 implicit-def $eflags
bb.1:
liveins: $eax
INLINEASM &"", 1, implicit $eax
...
Even simpler:
---
name: dead_register_def_pressure_tracking_underflow_assert_onereg
tracksRegLiveness: true
body: |
bb.0:
INLINEASM &"", 1 /* sideeffect attdialect */
INLINEASM &"", 1 /* sideeffect attdialect */
INLINEASM &"", 1 /* sideeffect attdialect */
INLINEASM &"", 1 /* sideeffect attdialect */
INLINEASM &"", 1 /* sideeffect attdialect */
INLINEASM &"", 1 /* sideeffect attdialect */
$eax = MOV32r1 implicit-def $eflags
$eax = MOV32r0 implicit-def $eflags
bb.1:
liveins: $eax
INLINEASM &"", 1, implicit $eax
...
This passes when the dead flag is applied to the first $eax def. (actually it passes with a dead on the second one too, where we have a missing verifier error). The pressure diff is negative without the dead flag for the SU for the first mov. There's no pressure diff with the dead flag.
I've stared at this for an extremely long time, and I'm stuck. It seems to me the logic of upward tracking is essentially faulty. It's implicitly relying on accurate dead flags. It assumes if it sees a live def, it will be used before a redef. There is detectDeadDefs, but it isn't always used. It also tries to use cached regunit results, so you cannot rely on that either. I'm also not sure why this is using LiveIntervals. If it's stepping backwards over every instruction, it should have exact liveness information.
I wanted to look into whether more reliable dead flag preservation would help the original testcase, but I can't reproduce with the patch reapplied anymore
Can we actually rely on accurate PDiffs for PhysRegs? https://github.com/llvm/llvm-project/blob/7a28192ce1c1d9d0398348eabc46c94eadb317d8/llvm/lib/CodeGen/MachineScheduler.cpp#L1364
Can hack things to repro the PDiff crash for AMDGPU by using standard scheduler
ScheduleDAGInstrs *MachineScheduler::createMachineScheduler() {
/*
// Select the scheduler, or set the default.
MachineSchedRegistry::ScheduleDAGCtor Ctor = MachineSchedOpt;
if (Ctor != useDefaultMachineSched)
return Ctor(this);
// Get the default scheduler set by the target for this function.
ScheduleDAGInstrs *Scheduler = PassConfig->createMachineScheduler(this);
if (Scheduler)
return Scheduler;
// Default to GenericScheduler.
*/
return createGenericSchedLive(this);
}
# RUN: llc -mtriple=amdgcn -run-pass=machine-scheduler -misched-bottomup -verify-misched -o - %s
---
name: pdiff
tracksRegLiveness: true
body: |
bb.0:
INLINEASM &"", 1 /* sideeffect attdialect */
INLINEASM &"", 1 /* sideeffect attdialect */
$vgpr1 = V_MOV_B32_e32 0, implicit-def $vgpr2, implicit $exec
$vgpr1 = V_MOV_B32_e32 0, implicit-def $vgpr2, implicit $exec
...
After 0e46b49de43349f8cbb2a7d4c6badef6d16e31ae ("Reapply "RegisterCoalescer: Add implicit-def of super register when coalescing SUBREG_TO_REG") I am seeing an assertion failure when building https://github.com/freebsd/freebsd-src/blob/main/sys/dev/fb/vga.c:
Reduced test case (C):
Further reduced with bugpoint: