Page MenuHomePhabricator

[SVE][InstrFormats] Explcitly set hasSideEffects for all SVE instructions.
ClosedPublic

Authored by paulwalker-arm on Thu, Jan 19, 8:04 AM.

Details

Summary

The instruction property hasSideEffects relies on the presence of
tablegen isel patterns when constructing its value, unless
specifically overriden. Since adding SVE scheduling information
we've noticed this property flip-flop as isel patterns have been
updated. To make things consistent (and correct) this patch
explicitly sets the property for all SVE instructions.

This has resulted in the following notable changes:

  • Normal load and store instructions no longer report having side effects.
  • All prefetch instructions correctly report having side effects.
  • FFR related instructions continue to report having side effects. This is likely overkill but I've chosen to remain cautious here.
  • Most all integer instructions no longer report having side effects.
  • Most all floating point instructions no longer report having side effects, but do now report their potential for raising FP exceptions. I do not know how to test the latter so I've again took a caution route of taging all floating point instructions except for DUPs.
  • The conflict detection intrinsics now report they don't touch memory.
NOTE: SVE isel makes significant use of psuedo instructions but this patch makes no effort to update them.
NOTE: We'll need a similar patch for SME but without a scheduling model it'll be harder to verify the results.

Diff Detail

Event Timeline

paulwalker-arm created this revision.Thu, Jan 19, 8:04 AM
Herald added a reviewer: efriedma. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
paulwalker-arm requested review of this revision.Thu, Jan 19, 8:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Jan 19, 8:04 AM

I cannot say I've double checked all the instructions but the output from the mca tests looks sensible and if nothing else I'm hoping this is a better starting point.

Matt added a subscriber: Matt.Thu, Jan 19, 10:08 PM
peterwaller-arm accepted this revision.EditedTue, Jan 24, 2:45 AM

I've been down the list of MCA output changes and agree those look good, and your reasoning seems good to me. Accepting as-is, since we think represents an improvement.

I believe the following command (adjusting relative paths) counts the number of instructions with hasSideEffects unset.

bin/llvm-tblgen -I ../../llvm/include -I ../../llvm/lib/Target/AArch64 ../../llvm/lib/Target/AArch64/AArch64.td --dump-json --class=AArch64Inst | \
  jq 'with_entries(select(.value | (type == "object" and has("hasSideEffects") and .hasSideEffects == null))) | length'

With this patch applied (on 5a7f47cc021b) I see the number drop from 4667 to 2198.

(and if you replace | length with | keys it shows the instructions). I do see a fair number of instructions remaining that don't have it set which you might have intended to capture, but I haven't dug in. These are the top ten, aggregating by three-char prefix:

 46   "ADD",
 48   "FMI",
 51   "FMA",
 64   "SML",
 64   "UML",
 70   "BFM",
 86   "MOV",
 95   "FCV",
 96   "CPY",
102   "FML",
This revision is now accepted and ready to land.Tue, Jan 24, 2:45 AM
paulwalker-arm added a comment.EditedTue, Jan 24, 5:30 AM

Thanks for the top tip @peterwaller-arm . I've analysed the output from a local run and observed:
392 belong to SVE, with all being pseudo instructions (These are the instructions that contain _UNDEF or _ZERO).
896 belong to SME.
757 look like scalar or NEON related.

I'll create a follow on patch for the SVE pseudo instructions, but given there's no easy way to test it I think I'll leave SME until we're further alone with its code generation support.

dewen added a subscriber: dewen.Mon, Jan 30, 7:49 PM