Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AMDGPU] Assertion `(!LeaveBefore || Idx <= LeaveBefore) && "Interference"' failed #109294

Closed
jayfoad opened this issue Sep 19, 2024 · 10 comments · Fixed by #109439
Closed

[AMDGPU] Assertion `(!LeaveBefore || Idx <= LeaveBefore) && "Interference"' failed #109294

jayfoad opened this issue Sep 19, 2024 · 10 comments · Fixed by #109439

Comments

@jayfoad
Copy link
Contributor

jayfoad commented Sep 19, 2024

With this test case I get:

llc -x mir zz.txt -start-after unreachable-mbb-elimination
llc: /home/jayfoad2/git/llvm-project/llvm/lib/CodeGen/SplitKit.cpp:1661: void llvm::SplitEditor::splitLiveThroughBlock(unsigned int, unsigned int, llvm::SlotIndex, unsigned int, llvm::SlotIndex): Assertion `(!LeaveBefore || Idx <= LeaveBefore) && "Interference"' failed.
@llvmbot
Copy link
Member

llvmbot commented Sep 19, 2024

@llvm/issue-subscribers-backend-amdgpu

Author: Jay Foad (jayfoad)

With [this test case](https://github.com/user-attachments/files/17061455/zz.txt) I get: ``` llc -x mir zz.txt -start-after unreachable-mbb-elimination llc: /home/jayfoad2/git/llvm-project/llvm/lib/CodeGen/SplitKit.cpp:1661: void llvm::SplitEditor::splitLiveThroughBlock(unsigned int, unsigned int, llvm::SlotIndex, unsigned int, llvm::SlotIndex): Assertion `(!LeaveBefore || Idx <= LeaveBefore) && "Interference"' failed. ```

@jayfoad
Copy link
Contributor Author

jayfoad commented Sep 19, 2024

The same assertion failed in #87721.

@jayfoad
Copy link
Contributor Author

jayfoad commented Sep 19, 2024

I tried reducing the MIR with llvm-reduce without success.

I tried running with -verify-regalloc but it failed like this, which I suspect is unrelated:

*** Bad machine code: Operand has incorrect register class. ***
- function:    test
- basic block: %bb.146  (0x60b5c40) [29456B;29808B)
- instruction: 29640B	SI_SPILL_V128_SAVE %8155:vreg_128, %stack.0, $sp_reg, 0, implicit $exec :: (store (s128) into %stack.0, align 4, addrspace 5)

*** Bad machine code: Illegal physical register for instruction ***
- function:    test
- basic block: %bb.146  (0x60b5c40) [29456B;29808B)
- instruction: 29640B	SI_SPILL_V128_SAVE %8155:vreg_128, %stack.0, $sp_reg, 0, implicit $exec :: (store (s128) into %stack.0, align 4, addrspace 5)
- operand 2:   $sp_reg
$sp_reg is not a SReg_32 register.

@arsenm
Copy link
Contributor

arsenm commented Sep 20, 2024

I tried reducing the MIR with llvm-reduce without success.

I tried running with -verify-regalloc but it failed like this, which I suspect is unrelated:

*** Bad machine code: Operand has incorrect register class. ***
- function:    test
- basic block: %bb.146  (0x60b5c40) [29456B;29808B)
- instruction: 29640B	SI_SPILL_V128_SAVE %8155:vreg_128, %stack.0, $sp_reg, 0, implicit $exec :: (store (s128) into %stack.0, align 4, addrspace 5)

*** Bad machine code: Illegal physical register for instruction ***
- function:    test
- basic block: %bb.146  (0x60b5c40) [29456B;29808B)
- instruction: 29640B	SI_SPILL_V128_SAVE %8155:vreg_128, %stack.0, $sp_reg, 0, implicit $exec :: (store (s128) into %stack.0, align 4, addrspace 5)
- operand 2:   $sp_reg
$sp_reg is not a SReg_32 register.

Yes, this is just a register definition issue (and we should probably just get rid of the sp_reg pseudo register at this point)

jayfoad added a commit to jayfoad/llvm-project that referenced this issue Sep 20, 2024
PRs llvm#69924 and llvm#72140 modified SIInstrInfo::isBasicBlockPrologue to skip
over EXEC modifications and spills when allocating VGPRs. But treating
VGPR spills as part of the prologue can confuse the register allocator
as in llvm#109294, so restrict it to SGPR spills, which were inserted during
SGPR allocation which is done in an earlier pass.

Fixes: llvm#109294
Fixes: SWDEV-485841
@arsenm
Copy link
Contributor

arsenm commented Sep 21, 2024

I can take a stab at reducing the MIR, I still have a number of patches that never made it upstream. I forgot that I never pushed the block reduction for instance

@arsenm
Copy link
Contributor

arsenm commented Sep 21, 2024

$sp_reg is not a SReg_32 register.

Yes, this is just a register definition issue (and we should probably just get rid of the sp_reg pseudo register at this point)

The problem here is also avoidable, too much of the machineFunctionInfo was deleted. Needs to set sp_reg to the real value

@jayfoad
Copy link
Contributor Author

jayfoad commented Sep 23, 2024

The problem here is also avoidable, too much of the machineFunctionInfo was deleted. Needs to set sp_reg to the real value

Thanks. I should not have deleted the definition of stackPtrOffsetReg. Updated test case: zz.txt

@arsenm
Copy link
Contributor

arsenm commented Sep 23, 2024

I've made some progress reducing this, but this is a tough one. It's another case where it depends on the exact slot indexes matter, so you have to run multiple passes to avoid the fresh LiveIntervals

@arsenm
Copy link
Contributor

arsenm commented Sep 23, 2024

Still really big issue109294.mir.zip

@arsenm
Copy link
Contributor

arsenm commented Sep 24, 2024

I think this is so sensitive because the SIMachineFunctionFields used for WWM spilling are still not serialized. We're losing reserved registers by resuming the compile

dhernandez0 added a commit to ROCm/rocMLIR that referenced this issue Sep 27, 2024
Update comment regarding barrier gfx908 hack: revert scheduling region when llvm/llvm-project#109294 is fixed
jayfoad added a commit that referenced this issue Sep 30, 2024
…gue (#109439)

PRs #69924 and #72140 modified SIInstrInfo::isBasicBlockPrologue to skip
over EXEC modifications and spills when allocating VGPRs. But treating
VGPR spills as part of the prologue can confuse the register allocator
as in #109294, so restrict it to SGPR spills, which were inserted during
SGPR allocation which is done in an earlier pass.

Fixes: #109294
Fixes: SWDEV-485841
xgupta pushed a commit to xgupta/llvm-project that referenced this issue Oct 4, 2024
…gue (llvm#109439)

PRs llvm#69924 and llvm#72140 modified SIInstrInfo::isBasicBlockPrologue to skip
over EXEC modifications and spills when allocating VGPRs. But treating
VGPR spills as part of the prologue can confuse the register allocator
as in llvm#109294, so restrict it to SGPR spills, which were inserted during
SGPR allocation which is done in an earlier pass.

Fixes: llvm#109294
Fixes: SWDEV-485841
qiaojbao pushed a commit to GPUOpen-Drivers/llvm-project that referenced this issue Oct 31, 2024
PRs llvm#69924 and llvm#72140 modified SIInstrInfo::isBasicBlockPrologue to skip
over EXEC modifications and spills when allocating VGPRs. But treating
VGPR spills as part of the prologue can confuse the register allocator
as in llvm#109294, so restrict it to SGPR spills, which were inserted during
SGPR allocation which is done in an earlier pass.

Fixes: llvm#109294
Fixes: SWDEV-485841
Change-Id: I328fb2edfca8110ea36c94812e60bc1d7663c266
xinyazhang added a commit to ROCm/aotriton that referenced this issue Nov 13, 2024
The Kernel Storage V2 achieves the following goals

Major changes:

* Move GPU kernel image to separate files​
    + Now organized as `aotriton.images/<vendor>-<arch>/<kernel_family>/<kernel_name>/FONLY__<functionals>___<GPU>.aks2'`
        - `<GPU>` can be a family of GPUs, e.g., `MI300X/MI300A/MI325X`
    + This enables per-architecture delivery
    + No more linking errors when binaries are bloated.
* Introduce `AKS2` file format to compress GPU kernels with LZMA. Reduce
the total package down to 200MB (MI200/300/Navi31)
    + AKS2 means "Aotriton Kernel Storage version 2"
    + LZMA is picked over Zstandard for much better compression ratio (~7%
vs ~12%) and acceptable performance (<0.1s)
* Look up kernel image relative to the `.so` file (achieved through
`dladdr`)
* Can only build the C++ part by setting `cmake` option
`AOTRITON_NOIMAGE_MODE` to `ON`

Minor changes:
* Release GIL in pybind11
* `aks2.py` tool is added to create `AKS2` file.
* Update Triton compiler to Oct/23/2024
    + This is to fix bug ```Assertion `(!LeaveBefore || Idx <= LeaveBefore)
&& "Interference"' failed```
    + See llvm/llvm-project#109294 for more
details
searlmc1 pushed a commit to ROCm/llvm-project that referenced this issue Nov 22, 2024
…gue (llvm#109439)

PRs llvm#69924 and llvm#72140 modified SIInstrInfo::isBasicBlockPrologue to skip
over EXEC modifications and spills when allocating VGPRs. But treating
VGPR spills as part of the prologue can confuse the register allocator
as in llvm#109294, so restrict it to SGPR spills, which were inserted during
SGPR allocation which is done in an earlier pass.

Fixes: llvm#109294
Fixes: SWDEV-485841

Change-Id: Ice1ab75074aa380c13e07c452a2854f78ff37ce7
arsenm added a commit that referenced this issue Dec 2, 2024
…plicit_def

Previously this would delete the IMPLICIT_DEF and not introduce the undef
flag on the use operand.

Fixes sub-issue found while reducing #109294
arsenm added a commit that referenced this issue Dec 2, 2024
…plicit_def

Previously this would delete the IMPLICIT_DEF and not introduce the undef
flag on the use operand.

Fixes sub-issue found while reducing #109294
arsenm added a commit that referenced this issue Dec 2, 2024
…plicit_def (#118321)

Previously this would delete the IMPLICIT_DEF and not introduce the undef
flag on the use operand.

Fixes sub-issue found while reducing #109294
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants