This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Add Missing v8.8a ALLINT PSTATE
AbandonedPublic

Authored by lenary on Aug 8 2022, 3:45 AM.

Details

Summary

I noticed, on close reading of the Arm ARM, that llvm is missing
assembly support for a recently-added PSTATE: ALLINT from FEAT_NMI.

This patch:

  • Adds the missing PSTATE
  • Aligns the decoder more closely with the Arm ARM Decoder pseudocode, including allowing 4-bit immediates for all PSTATES except those explicitly noted (ALLINT, and the SVCRs that we handle separately).
  • Tries to centralise the information about the immediates accepted by a given named PSTATE, rather than spreading it around the LLVM codebase.
  • Updates tests for existing pstates to reflect these changes.
  • Adds another missing system register, from GIC v3.3. It is enabled with FEAT_NMI because it's from FEAT_GICv3_NMI which implies FEAT_NMI.

Parts of this patch are by David Candler and Tomas Matheson.

Diff Detail

Event Timeline

lenary created this revision.Aug 8 2022, 3:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2022, 3:45 AM
lenary requested review of this revision.Aug 8 2022, 3:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2022, 3:45 AM
lenary added inline comments.Aug 8 2022, 7:26 AM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
3147

This is using the wrong accessor. I will update the patch with the right one after the first set of feedback comes in.

lenary planned changes to this revision.Aug 8 2022, 9:07 AM

Sorry, more changes are needed, it turns out. I've missed at least one relevant sysreg.

lenary requested review of this revision.Aug 19 2022, 9:04 AM
lenary updated this revision to Diff 454038.
lenary edited the summary of this revision. (Show Details)

A reference I found helpful when looking at this in revision ia of the Arm ARM (https://developer.arm.com/documentation/ddi0487/ia/?lang=en) Instructions for accessing the PSTATE fields

CFINV ; Inverts the value of PSTATE.C
MSR DAIFSet, #Imm4 ; Used to set any or all of DAIF to 1
MSR DAIFClr, #Imm4 ; Used to clear any or all of DAIF to 0
MSR SPSel, #Imm4 ; Used to select the Stack Pointer, between SP_EL0 and SP_ELx
MSR UAO, #Imm4 ; Used to set the value of PSTATE.UAO
MSR PAN, #Imm4 ; Used to set the value of PSTATE.PAN
MSR DIT, #Imm4 ; Used to set the value of PSTATE.DIT
MSR SSBS, #Imm4 ; Used to set the value of PSTATE.SSBS
MSR TCO, #Imm4 ; Used to set the value of PSTATE.TCO
MSR ALLINT, #Imm1 ; Used to set the value of PSTATE.ALLINT

I think the disassembler changes to decode the field as 4-bits looks fine. Changing the assembler to accept 4-bit fields for the fields that are logically on or off, while closer to the architecture, makes it possible to write assembler that isn't going to assemble on the GNU assembler which only accepts 0 or 1. I think that if we change we should at least inform GNU of the change. Personally I don't think that any human will choose to use anything other than 0 or 1 so I don't necessarily think this is going to be a problem in practice.

Hope I've understood that correctly.

tmatheson accepted this revision.Sep 26 2022, 2:05 AM

Couple of suggestions but otherwise LGTM.

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
1125

Please write the type name explicitly where auto is not obvious.

llvm/test/MC/AArch64/armv8.1a-pan.s
18

Maybe keep the tests with non-0/1 but valid immediates.

This revision is now accepted and ready to land.Sep 26 2022, 2:05 AM

A reference I found helpful when looking at this in revision ia of the Arm ARM (https://developer.arm.com/documentation/ddi0487/ia/?lang=en) Instructions for accessing the PSTATE fields

CFINV ; Inverts the value of PSTATE.C
MSR DAIFSet, #Imm4 ; Used to set any or all of DAIF to 1
MSR DAIFClr, #Imm4 ; Used to clear any or all of DAIF to 0
MSR SPSel, #Imm4 ; Used to select the Stack Pointer, between SP_EL0 and SP_ELx
MSR UAO, #Imm4 ; Used to set the value of PSTATE.UAO
MSR PAN, #Imm4 ; Used to set the value of PSTATE.PAN
MSR DIT, #Imm4 ; Used to set the value of PSTATE.DIT
MSR SSBS, #Imm4 ; Used to set the value of PSTATE.SSBS
MSR TCO, #Imm4 ; Used to set the value of PSTATE.TCO
MSR ALLINT, #Imm1 ; Used to set the value of PSTATE.ALLINT

I think the disassembler changes to decode the field as 4-bits looks fine. Changing the assembler to accept 4-bit fields for the fields that are logically on or off, while closer to the architecture, makes it possible to write assembler that isn't going to assemble on the GNU assembler which only accepts 0 or 1. I think that if we change we should at least inform GNU of the change. Personally I don't think that any human will choose to use anything other than 0 or 1 so I don't necessarily think this is going to be a problem in practice.

Hope I've understood that correctly.

I think you have.

I have informed our binutils maintainers, but they're not receptive to updating their implementation. I think we should press ahead anyway.

llvm/test/MC/AArch64/armv8.1a-pan.s
18

these are the "invalid value" tests, but yes, I should have added a check for msr pan, #15. will do when I commit.

lenary abandoned this revision.Nov 22 2022, 7:14 AM

Due to changes in v8.9a/v9.4a, this patch has got more complex and we'll be starting over with the changes. The new patch will come some time this week.