This adds some more scalar instructions that are both associative and commutative to isAssociativeAndCommutative, allowing the machine combiner to reassociate them to reduce critical path length.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
From the tests it's not immediately clear what is improved. Have you run any benchmarks to see what improves?
The swift-return test shows it best, where it changes:
; CHECK: add [[TMP:x.*]], x0, x1 ; CHECK: add [[TMP]], [[TMP]], x2 ; CHECK: add [[TMP]], [[TMP]], x3 ; CHECK: add x0, [[TMP]], x4
To:
; CHECK: add [[TMP:x.*]], x0, x1 ; CHECK: add [[TMP2:x.*]], x2, x3 ; CHECK: add [[TMP]], [[TMP]], [[TMP2]] ; CHECK: add x0, [[TMP]], x4
This allow the first two operations to be executed in parallel. This reassociation in the machine combiner is what this patch now allows.
I see this has just been reverted but I'd like to add that we have also seen failures, that I've bisected down to this patch. See https://ci.chromium.org/ui/p/fuchsia/builders/prod/clang-linux-arm64/b8797325013179537905/overview, which is a 2 stage clang build. It causes an unrelated test to fail, presumably because of a miscompile.
Oh OK, Sorry for the breakage. I would guess it was the ands, that probably doesn't work as I expect it to. It looks like the buildbot went green again a couple of times after this patch, I guess it doesn't always run all the tests?
I'll see if I can reproduce the error. Thanks for the reports and the revert.
After testing it, I've recommitted this without ANDS and the bot renamed green. Please let me know if any other issues come up.