dmgreen added inline comments.
================
Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:1495
let DecoderNamespace = "Fallback";
+ let Defs = [NZCV];
}
----------------
stelios-arm wrote:
> SjoerdMeijer wrote:
> > 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 .
> I will talk with @dmgreen and if it is decided that it is needed to change, I
> will address it in a future revision. Please note that the next revision of
> patch, will not address this comment (temporarily).
Hmm. I feel like adding the pseudo is more trouble than it is worth - they do
not come for free. With the extra lowering and adding things like scheduling
info, etc. It feels simpler to me to set the flags, even if that's not always
exactly what the instruction would do.
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
