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

[Stelios Ioannou via Phabricator via cfe-commits]

stelios-arm marked 4 inline comments as done.
stelios-arm added inline comments.


================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:363
 
+  if (HasRandGen)
+    Builder.defineMacro("__ARM_FEATURE_RNG", "1");
----------------
SjoerdMeijer wrote:
> Where/when is `HasRandGen` set?
Oh, I forgot to set it, I am going to address this in the new revision. 


================
Comment at: clang/test/Preprocessor/init-aarch64.c:28
 // AARCH64-NEXT: #define __ARM_FEATURE_NUMERIC_MAXMIN 1
+// AARCH64-NEXT: #define  __ARM_FEATURE_RNG 1
 // AARCH64-NEXT: #define __ARM_FEATURE_UNALIGNED 1
----------------
SjoerdMeijer wrote:
> Why are we expecting this here? Are we not only expecting this for v8.5?
> 
> We also need a negative test and CHECK-NOT of this where we don't expect this.
Correct this shouldn't be here. I am going to address this in a new revision. 


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrFormats.td:1495
   let DecoderNamespace = "Fallback";
+  let Defs = [NZCV];
 }
----------------
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). 


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.td:1274
+def : Pat<(AArch64mrs imm:$id),
+          (MRS imm:$id)>;
+
----------------
dmgreen wrote:
> SjoerdMeijer wrote:
> > Nit: can be on the same line.
> I always prefer Pat's to have input and output lines separate, for what it's worth. It makes them more easily readable.
The styling for Pat's is not consistent across the file, some are in one line and others have the input and output lines separate. I also prefer the latter for readability. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98264/new/

https://reviews.llvm.org/D98264