[PATCH] D98264: [AArch64] Implement __rndr, __rndrrs intrinsics

[Dave Green via Phabricator via cfe-commits]

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