SjoerdMeijer added inline comments.
================
Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:1495
let DecoderNamespace = "Fallback";
+ let Defs = [NZCV];
}
----------------
dmgreen wrote:
> 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.
Okidoki, then we at least need some comments here explaining this all.
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
