This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SME] Disable GlobalISel/FastISel for SME functions.
ClosedPublic

Authored by sdesmalen on Oct 20 2022, 7:47 AM.

Details

Summary

This patch ensures that GlobalISel and FastISel fall back to regular DAG ISel when:

  • A function requires streaming-mode to be enabled at the start/end of the function. This happens when the function has no streaming interface, but does have a streaming body.
  • A function requires a lazy-save to be committed at the start of the function. This happens if the function has the aarch64_pstate_za_new attribute.
  • A call to a function requires a change in streaming-mode.
  • A call to a function requires a lazy-save buffer to be set up.

Patch by @CarolineConcatto

Diff Detail

Event Timeline

sdesmalen created this revision.Oct 20 2022, 7:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2022, 7:47 AM
sdesmalen requested review of this revision.Oct 20 2022, 7:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2022, 7:47 AM
david-arm added inline comments.Oct 27 2022, 11:52 AM
llvm/lib/Target/AArch64/AArch64FastISel.cpp
5121

Should we also check for arm_new_za here too?

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
22063

I guess this covers most cases, except I'm not sure what happens at -O0 if the caller is in a shared-ZA function and contains something like a 128-bit FP IR that ends up getting lowered to a library call to a private-ZA function. Maybe this just works and we just a need a test for it?

sdesmalen updated this revision to Diff 471984.Oct 31 2022, 6:28 AM

Added two new test-cases to ensure that f128 add is lowered with the expected streaming-mode toggle and/or ZA lazy-save.

llvm/lib/Target/AArch64/AArch64FastISel.cpp
5121

I don't believe so, because the work for arm_new_za functions is done by the SMEABI pass at the LLVM IR level.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
22063

I've added two tests for this scenario, which seem to exhibit the desired behaviour.

Thanks for adding the new tests @sdesmalen! I just had a question about @za_new_caller_to_za_shared_callee - perhaps I'm missing something obvious and this is my :face_palm moment!

llvm/test/CodeGen/AArch64/sme-disable-gisel-fisel.ll
181

nit: s/streaminhg/streaming/

252

Not sure if this is caused by your patch or not, but it looks a bit odd. Perhaps I'm missing something, but it seems to be treating za_shared_callee as a private-ZA function and setting up a lazy-save + restore.

sdesmalen updated this revision to Diff 472251.Nov 1 2022, 3:42 AM

Fixed typo in test that set an invalid attribute, i.e.

s/aarch64_pstate_sm_shared/aarch64_pstate_za_shared/
sdesmalen marked 2 inline comments as done.Nov 1 2022, 3:42 AM
sdesmalen added inline comments.
llvm/test/CodeGen/AArch64/sme-disable-gisel-fisel.ll
252

That was because za_shared_callee had aarch64_pstate_sm_shared instead of aarch64_pstate_za_shared as it's attribute. Thanks for pointing out!

david-arm added inline comments.Nov 3 2022, 2:49 AM
llvm/test/CodeGen/AArch64/sme-disable-gisel-fisel.ll
229

I'm not sure why the CHECK-FISEL and CHECK-GISEL lines are different? The GISEL code looks correct, since we're setting up the lazy save buffer, although in practice it's not needed.

sdesmalen updated this revision to Diff 474025.Nov 8 2022, 9:09 AM
sdesmalen marked 4 inline comments as done.

Added a condition not to use FastISel when the function has ZA state.

llvm/lib/Target/AArch64/AArch64FastISel.cpp
5121

I found that this caused the discrepancy between FastISel and GlobalISel in that test, because it wouldn't try to set up a lazy-save buffer. So it turns out we do need that check for ZA state here.

david-arm accepted this revision.Nov 9 2022, 7:47 AM

Bellissimo! LGTM. :)

This revision is now accepted and ready to land.Nov 9 2022, 7:47 AM
This revision was landed with ongoing or failed builds.Nov 9 2022, 8:11 AM
This revision was automatically updated to reflect the committed changes.