SjoerdMeijer added inline comments.
================
Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:1495
let DecoderNamespace = "Fallback";
+ let Defs = [NZCV];
}
----------------
SjoerdMeijer wrote:
> dmgreen wrote:
> > 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.
> > I believe that would have an over-lapping definition with the existing MRS
> > instruction?
>
> Ah yeah, that might be true.
>
> > It would need to be a pseudo I think
>
> How much work is adding a pseudo for this case? My first reaction would be
> just trying to model this correctly, then we potentially don't have to worry
> again later about this.
I just wanted to add that I don't have too strong opinions on this, but what I
suggested seemed more the correct, even though the consequence of setting NCZV
for all MRS is minimal. So I will leave this one up to you @dmgreen and
@stelios-arm .
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
