When already have (op N0, N2), reassociate (op (op N0, N1), N2) to (op (op N0, N2), N1) to reuse the exist (op N0, N2)
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/test/CodeGen/X86/masked-iv-safe.ll | ||
---|---|---|
269 ↗ | (On Diff #418434) | This seems regression. The same below. |
llvm/test/CodeGen/X86/masked-iv-safe.ll | ||
---|---|---|
269 ↗ | (On Diff #418434) | Yeah, this change brings some regressions because of two reasons:
But I think they should be fixed on every backend's isReassocProfitable. I prefer to fix these on individual changes. |
llvm/test/CodeGen/X86/masked-iv-safe.ll | ||
---|---|---|
269 ↗ | (On Diff #418434) | We shouldn't commit a patch with any known regressions. An individual fix is fine, but it should be ahead of this one so that we won't bring the regression at any time. |
A few style comments
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
---|---|---|
1022–1024 | if (auto *C1 = dyn_cast<ConstantSDNode>(N0.getOperand(1))) { | |
1101 | if (auto *GA = dyn_cast<GlobalAddressSDNode>(N0.getOperand(1))) | |
1108 | (style) Early-out: auto *LoadStore = dyn_cast<MemSDNode>(Node); if (!LoadStore) return false; |
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
---|---|---|
1062 | auto *LoadStore |
LGTM - with one more style minor
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
---|---|---|
1037 | if (auto *LoadStore = dyn_cast<MemSDNode>(Node);) |
Hi @bcl5980, one of our internal tests started to hit an assertion failure while building which I bisected back to this change. I have filed the details as issue #56170, can you take a look?
This somehow caused a regression in the wasm targets, which I bisected to here. Certain files when compiled with -O1 appear to hang for a very long time (maybe forever). This happened on the Bullet physics engine codebase in the Emscripten test suite, but affects all wasm targets (wasm32-unknown-unknown, -wasi, and -emscripten).
This doesn't affect other targets, which I guess is why it wasn't noticed before it reached our CI. So maybe this uncovered an existing wasm bug.
@tlively is reducing this atm. Posting the original testcase here for future reference.
STR: clang++ -target wasm32-unknown-unknown -O1 -c bug.cpp. That should take less than a second normally, but from this change landing it hangs a very long time (I measured at least 10 seconds during bisection).
Thanks for the case. I try to minimize the test:
target triple = "wasm32-unknown-unknown" define float @reassociate_xor(float %x, float %y) { entry: ; preds = %if.then, %entry %cmp0 = fcmp ule float %x, 0x3E80000000000000 %cmp1 = fcmp ugt float %y, 0x3E80000000000000 %cmp2 = xor i1 %cmp0, %cmp1 br i1 %cmp2, label %if.end.i, label %if.then.i if.then.i: ; preds = %if.end br label %if.end.i if.end.i: ; preds = %if.then.i, %if.end %s = phi i32 [ 1, %entry ], [ 0, %if.then.i ] ret float %x }
It looks rebuildSetCC will visitXor in a loop keep the original dag before reassoicate cause deadloop.
I will continue work on it to find a solution.
llvm/test/Transforms/Reassociate/pr56170.ll | ||
---|---|---|
4 ↗ | (On Diff #439631) | Actually almost every target can trigger this issue. Move the test to x86 is OK for me. |