We hit the infinite loop reported by https://bugs.llvm.org/show_bug.cgi?id=46877 and the reason is the same as D77319. So we need to remove the dead node we created to avoid the sideeffect on DAGCombiner.
Details
- Reviewers
RKSimon spatel efriedma arsenm richard-uhler hans qiucf - Group Reviewers
Restricted Project - Commits
- rG960cbc53ca17: [DAGCombine] Remove dead node when it is created by getNegatedExpression
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
If it avoids an infinite loop, it can't be an NFC change. Is not possible to reduce the original test case?
The original test case is too big to get something useful. I happened to grab a basic block from it. It's not small enough to be a test, but it would be much easier to analyze: https://reviews.llvm.org/P8230
The test shows that we are not actually infinite looping; we are exponentially increasing compile time with each additional set of FMA combines.
If I limit the live code in the test by changing the final store from "store float %288, float* %80, align 4" and time it:
store float %233, float* %80, align 4 --> "time llc" --> user 0m30.640s store float %229, float* %80, align 4 --> "time llc" --> user 0m19.956s store float %224, float* %80, align 4 --> "time llc" --> user 0m10.881s store float %217, float* %80, align 4 --> "time llc" --> user 0m5.750s
I don't know if that changes the solution or the potential test. Maybe we could add or check some debug statement to track the number of nodes created?
Thank you for doing this! The test is small enough to demonstrate the problem I think.
The test shows that we are not actually infinite looping; we are exponentially increasing compile time with each additional set of FMA combines.
If I limit the live code in the test by changing the final store from "store float %288, float* %80, align 4" and time it:
store float %233, float* %80, align 4 --> "time llc" --> user 0m30.640s
store float %229, float* %80, align 4 --> "time llc" --> user 0m19.956s
store float %224, float* %80, align 4 --> "time llc" --> user 0m10.881s
store float %217, float* %80, align 4 --> "time llc" --> user 0m5.750s
I don't know if that changes the solution or the potential test. Maybe we could add or check some debug statement to track the number of nodes created?
Good catch. And it also explains more clear on why the fix works as we are removing the unnecessary dead nodes we created that increase the problem size of the DAGCombine. As the final DAG is the same w/o the fix and I am worried the test is easy to be broken if we check the temp node we created, I tend to have chaofang's test to verify we are not increasing the compiling time. Does it make sense ?
And thank you for all your help on the tests!
LGTM
llvm/test/CodeGen/X86/pr46877.ll | ||
---|---|---|
2 | I think if we are going to spend the time to execute a test, we might as well use FileCheck to verify the output asm. That way, we can detect if some future patch is doing something unexpected. |
llvm/test/CodeGen/X86/pr46877.ll | ||
---|---|---|
2 | ok,I will update the test. |
Pushed to 11.x as 4d16d8dfe50eb45545e844c3c9acafd363637dad
Please let me know if there are any follow-ups.
Hi,
I wrote https://bugs.llvm.org/show_bug.cgi?id=49393 that seems to start happening with this patch.
It seems that, craig is working on this bugzallia issue. Thank you for reporting this.
I think if we are going to spend the time to execute a test, we might as well use FileCheck to verify the output asm. That way, we can detect if some future patch is doing something unexpected.