This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Add GPR rr instructions to isAssociativeAndCommutative
ClosedPublic

Authored by dmgreen on Sep 20 2022, 1:13 AM.

Details

Summary

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.

Diff Detail

Event Timeline

dmgreen created this revision.Sep 20 2022, 1:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2022, 1:13 AM
dmgreen requested review of this revision.Sep 20 2022, 1:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2022, 1:13 AM

From the tests it's not immediately clear what is improved. Have you run any benchmarks to see what improves?

labrinea accepted this revision.Sep 20 2022, 3:29 AM

Looks okay.

This revision is now accepted and ready to land.Sep 20 2022, 3:29 AM

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.

This revision was landed with ongoing or failed builds.Nov 16 2022, 4:39 AM
This revision was automatically updated to reflect the committed changes.
vitalybuka reopened this revision.Nov 22 2022, 11:03 AM
This revision is now accepted and ready to land.Nov 22 2022, 11:03 AM

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.

Build bot was green only when I scheduled older revisons to find a root
cause.

After testing it, I've recommitted this without ANDS and the bot renamed green. Please let me know if any other issues come up.