This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SME] Disable remat of VL-dependent ops when function changes streaming mode.
ClosedPublic

Authored by sdesmalen on Aug 30 2023, 6:10 AM.

Details

Summary

This is a way to prevent the register allocator from inserting instructions
which behave differently for different runtime vector-lengths, inside a
call-sequence which changes the streaming-SVE mode before/after the call.

I've considered using BUNDLEs in Machine IR, but found that using this is
not possible for a few reasons:

  • Most passes don't look inside BUNDLEs, but some passes would need to look inside these call-sequence bundles, for example the PrologEpilog pass (to remove the CALLSEQSTART/END), a PostRA pass to remove COPY instructions, or the AArch64PseudoExpand pass.
  • Within the streaming-mode-changing call sequence, one of the instructions is a CALLSEQEND. The corresponding CALLSEQBEGIN (AArch64::ADJCALLSTACKUP) is outside this sequence. This means we'd end up with a BUNDLE that has [SMSTART, COPY, BL, ADJCALLSTACKUP, COPY, SMSTOP]. The MachineVerifier doesn't accept this, and we also can't move the CALLSEQSTART into the call sequence.

Maybe in the future we could model this differently by modelling
the runtime vector-length as a value that's used by certain operations
(similar to e.g. NCZV flags) and clobbered by SMSTART/MMSTOP, such that the
register allocator can consider these as actual dependences and avoid
rematerialization. For now we just want to address the immediate problem.

Diff Detail

Event Timeline

sdesmalen created this revision.Aug 30 2023, 6:10 AM
sdesmalen requested review of this revision.Aug 30 2023, 6:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2023, 6:10 AM
aemerson accepted this revision.Aug 30 2023, 7:40 AM

I have a slight concern that relying on the MFI's flag being set on a DAG codegen call is brittle, in case in future there are other pathways that change streaming mode. Scanning the function seems a more reliable way to set the flag, but for now this is ok with me.

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
8548

physical

This revision is now accepted and ready to land.Aug 30 2023, 7:40 AM
Matt added a subscriber: Matt.Aug 30 2023, 11:14 AM
sdesmalen updated this revision to Diff 555025.Aug 31 2023, 6:43 AM
sdesmalen marked an inline comment as done.

Use isVirtual() explicitly rather than relying on getRegClassOrNull().
(this also removed the need for the assert)

I have a slight concern that relying on the MFI's flag being set on a DAG codegen call is brittle, in case in future there are other pathways that change streaming mode. Scanning the function seems a more reliable way to set the flag, but for now this is ok with me.

Thanks for pointing out, I had a patch for that which I forgot to push. 8f469bfedc907a391da7207ba248afdb7e8c555d ensures that any streaming-mode changes are always done through the function changeStreamingMode.

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
8548

I've actually simplified it with an explicit check for isVirtual, rather than relying on getRegClassOrNull and then use getRegClass() instead.

paulwalker-arm accepted this revision.Aug 31 2023, 8:12 AM
paulwalker-arm added inline comments.
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
8555

Perhaps worth saying "Avoid rematerializing rematerializable instructions...". This might be obvious but for a second I nearly asked "Should this block also include the inc/dec instructions?"

This revision was landed with ongoing or failed builds.Sep 1 2023, 5:15 AM
This revision was automatically updated to reflect the committed changes.
sdesmalen marked an inline comment as done.