dmgreen added inline comments.
================ Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15993 + DAG.getConstant(0, dl, MVT::i32), + DAG.getConstant(AArch64CC::EQ, dl, MVT::i32), A.getValue(1)); + return DAG.getMergeValues( ---------------- Can you make sure EQ is correct here? I think it might be backwards. ================ Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:1495 let DecoderNamespace = "Fallback"; + let Defs = [NZCV]; } ---------------- SjoerdMeijer wrote: > dmgreen wrote: > > SjoerdMeijer wrote: > > > Do all MRS instructions do this? > > No, but some do and it's not obvious which ones do and don't. I think it > > should be reasonable to always def NZCV here, even if they are just dead. > > It should be very rare that it would be beneficial for NZCV to be live > > across a MRS instruction. > True, but since creating another definition is cheap, what would the cons be > of: > > class MRS_NZCV : MRSI { > .. > let Defs = [NZCV]; > } > > The way I look at it is that the description would be more accurate? I believe that would have an over-lapping definition with the existing MRS instruction? It would need to be a pseudo I think, which would eventually be turned into a MSR proper late enough on the pipeline for it to be valid (or the other way around, the no-nzcv version gets turned into a the nzcv version to keep the verifier happy). It could also be a optional def, but those are only used in the arm backend and I would not recommend using them anywhere else. I would probably suggest just setting MRS as a NZCV setting instruction, unless we find a reason to need to implement it differently. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98264/new/ https://reviews.llvm.org/D98264 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits