This change makes callees with the arm_preserves_za
type attribute comply with the dormant state requirements
when it's caller has the arm_shared_za type attribute
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
7399 | requiresLazySave should return true, even when the function has the aarch64_pstate_za_preserved attribute. That also makes this code below a bit simpler. | |
7411 | Streaming-properties are orthogonal to ZA, so don't belong in this condition. | |
llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.h | ||
81 | Could you keep the original name please? |
llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.h | ||
---|---|---|
81 | Can I do it as a separate change at least? It doesn't match the rest of the class's interface at all. |
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
7411 | It's possible for if (RequiresSMChange) PStateSM = getPStateSM(DAG, Chain, CallerAttrs, DL, MVT::i64); to call LowerCall an additional time and generate an extra redundant store into the TPIDR2 save slices parameter, which is why this is here. I will look for a better way of handling this though. |
llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.h | ||
---|---|---|
81 | The preserving ZA is not really an interface in the sense that the ABI defines e.g. a SharedZA/PrivateZA/Streaming interface. The 'interface' part of the name refers to the caller having to be aware of the callee's interface when generating code for the call. For example, a private-ZA interface requires the caller to set up the lazy-save mechanism when the caller has ZA state. The __arm_preserves_za attribute (or aarch64_pstate_za_preserved as it's named in LLVM IR) is more of a 'hint' to the compiler to generate more efficient code. It is a property that can be ignored by the compiler and the generated code would still be correct. Note that I pushed a change yesterday to rename hasNewZAInterface() in rG0a32a999aea07e373e1b3c9e6f0d20d3f7f1a7b8, because 'new ZA' is not part of the interface (externally visible to callers), but rather an implementation property of the function's body/definition. As such, I'd rather stick with preservesZA as the name of this interface. |
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
7387 | Am I reading this wrong, or should this have been named NumZaSaveSlicesAddr ? | |
7814–7850 | Can you write this as: if (RequiresLazySave) { if (!CalleeAttrs.preservesZA()) { // code to restore ZA } Result = DAG.getNode(...aarch64_sme_set_tpidr2...); } | |
llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.h | ||
38 | nit: maybe add a comment that we use this for SME ABI routines. | |
85 | nit: this interface seems redundant. |
llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp | ||
---|---|---|
37 | Perhaps it's worth adding a new SMEAttrs constructor, such that you can use that both here and in AArch64ISelLowering.cpp SMEAttrs(StringRef funcname) { /* handle SME ABI functions here */ } |
llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp | ||
---|---|---|
37 | What benefit is this supposed to give? If you just use a StringRef param in a constructor you will have to make many assumptions to satisfy the mutex asserts in set and probably just end up doing Bitmask |= Za_NoLazySave with whatever comes out of that constructor anyway. Isn't getting this functionality from AArch64ISelLowering.cpp is what getCalleeAttrsFromExternalFunction is providing anyway? |
llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp | ||
---|---|---|
37 | My suggestion is to avoid having to write this if (FuncName == "__arm_tpidr2_save" || .. code in multiple places.
What assumptions will you have to make? |
llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp | ||
---|---|---|
37 | In the end I realised what you meant and I uploaded another patch set with the StringRef constructor |
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp | ||
---|---|---|
7368 | Do we really need the if-statement? For example, can we just write: else if (auto *ES = ...) CalleeAttrs = SMEAttrs(ES->getSymbol()); If the default for SMEAttrs(<symbol>) is that it doesn't have any streaming- or ZA properties then this is equivalent to what we had before this patch | |
llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.cpp | ||
26–30 | Does this need a new assert to say that NewZA and SharedZA cannot be combined with NoLazySave? | |
llvm/lib/Target/AArch64/Utils/AArch64SMEAttributes.h | ||
90 | I'd rather we don't add an operator bool(), because it's not clear to me what it is supposed to represent that the existing interfaces don't represent already. |
llvm/test/CodeGen/AArch64/sme-lazy-save-call.ll | ||
---|---|---|
172 | Could you also add the "aarch64_pstate_sm_compatible" attribute so that the compiler must call __arm_sme_state() as part of the call-sequence and test that we don't set up the lazy-save mechanism for that __arm_sme_state() call? nit: Could you maybe also give this test a better name than foo? :) |
llvm/test/CodeGen/AArch64/sme-lazy-save-call.ll | ||
---|---|---|
172 | Done & nit done. |
LGTM
llvm/test/CodeGen/AArch64/sme-lazy-save-call.ll | ||
---|---|---|
216 | nit: can you add "aarch64_pstate_za_preserved" next to the declaration of @private_za_preserved_callee similar to how you've done it for @za_shared_caller_za_preserved_callee, and then remove this attribute? |
Do we really need the if-statement? For example, can we just write:
If the default for SMEAttrs(<symbol>) is that it doesn't have any streaming- or ZA properties then this is equivalent to what we had before this patch