[PATCH] D139925: [AArch64] Add new v9.4-A PM pstate system register

[Lucas Prates via Phabricator via llvm-commits]

pratlucas marked 2 inline comments as done.
pratlucas added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:1793
+  let DecoderMethod = "DecodeSystemPStateImm0_1Instruction";
   // MSRpstateI aliases with MSRI. When the MSRpstateI decoder method returns
   // Fail the decoder should attempt to decode the instruction as MSRI.
----------------
lenary wrote:
> pratlucas wrote:
> > pratlucas wrote:
> > > lenary wrote:
> > > > Do we have decoding tests for the case where you fallback from `MSRpstateImm0_15` to `MSRpstateImm0_1` to `MSRI`? This would presumably be a test where we use an unallocated MSR (imm) encoding?
> > > The fallback applies every time the decoder method fails due to the lack of the required feature for a particular pstate register.
> > > This is currently covered by the negative tests for `PAN`, `UAO`, `DIT`, `SSBS`, and `TCO`.
> > On a deeper look, this existing behaviour doesn't look like the best approach to handle those situations. `MSRI` ends up decoding the instruction using the register form of the `msr` instruction, generating a somewhat meaningless result.
> > I'll have a look into an alternative for that, but given that's existing behaviour the change may go into a separate patch.
> > I'll have a look into an alternative for that, but given that's existing behaviour the change may go into a separate patch.
> 
> As long as we can parse whatever we produce, I think it should be ok. Maybe worth checking what binutils prints in these cases?
Yes, I've double checked the results we produce and they are assembled back into the correct instruction.
Binutils doesn't check for which features are enabled and just disassembles the feature-specific register name. For unallocated/reserved encodings, binutils behaves the same way as llvm, so I think we are ok indeed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139925/new/

https://reviews.llvm.org/D139925