[Mips] mips1 DivByZeroTrap #81311
[Mips] mips1 DivByZeroTrap #81311
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
cc @wzssyqa |
| ; GP32-NEXT: teq $5, $zero, 7 | ||
| ; GP32-NEXT: jr $ra | ||
| ; GP32-NEXT: mflo $2 | ||
| ; GP32R0R1-LABEL: sdiv_i32: |
There was a problem hiding this comment.
Note, MIPS32R1 is not same as MIPS I.
The road map of MIPS Rev is like
32bit I II 32R1
64bit III IV V. 64R1
This name scheme is quite confusion.
There was a problem hiding this comment.
I based the naming here off the entries that already existed for MIPS II, my changes are specifically for MIPS I, not Mips32R1, though I agree the naming scheme is very confusing. Whoever wrote this originally referred to MIPSII as MIPS32R0R2, then MIPS32R1 as just MIPS32R1 Would you recommend changing the names for both mips1 and mips2?
There was a problem hiding this comment.
Since MIPS I is not supported well yet for LLVM, and I guess the 32R0 was supposed to be equal with MIPS II in this context.
If we need MIPS I, we may use a new name for it, maybe 32_I?
| ; GP32R0R2-NEXT: nop | ||
| ; GP32R0R2-NEXT: lw $ra, 20($sp) # 4-byte Folded Reload | ||
| ; GP32R0R2-NEXT: jr $ra | ||
| ; GP32R0R2-NEXT: addiu $sp, $sp, 24 |
There was a problem hiding this comment.
The only difference of GP32R0R2 and GP32R0R1 is about some nop?
If so, they can be merged.
There was a problem hiding this comment.
The nop is at least significant on mips1 since it has load delay slots, removing it would not be valid, but I agree that this test case is kind of pointless otherwise. or is there some way of merging it and keeping the nop only for mips1?
There was a problem hiding this comment.
Can you have a try this syntax?
GP32R_1-NEXT: nop
GP32-NEXT: lw
``
As you may notice that
FileCheck %s -check-prefixes=GP32,GP32R0R1
`FileCheck` can have multiple prefixes.
| ; GP32R2R5-NEXT: div $zero, $4, $5 | ||
| ; GP32R2R5-NEXT: teq $5, $zero, 7 | ||
| ; GP32R2R5-NEXT: jr $ra | ||
| ; GP32R2R5-NEXT: mflo $2 |
There was a problem hiding this comment.
R0R2 is same with R2R5?
If so, why not merge them?
There was a problem hiding this comment.
good point, these test cases were autogenerated by utils/update_llc_test_checks.py, I just changed the header bit. I can merge them, but if the test cases change again later, they'll have to be remerged.
|
Update? |
|
@cmccord-dev Ping. |
|
I did some more testing on another project I'm working on with this fix and found that it trips assertions on more complex examples. I don't currently have a good minimal example, but it seems I'm not handling liveness correctly with the newly generated blocks. If someone who understands the system better could take a look I would greatly appreciate it. Additionally, I've only tested with sdiv/udiv but I believe srem/urem should also be tested. I will implement these, along with the comments above when I get a chance. |
Inserts bnez + break instead of teq $zero for mips1 target
Fixes #80554
I updated the llvmir sdiv/udiv test as well.
This is my first contribution, please let me know if there is anything else I need to do!