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

[Dave Green via Phabricator via cfe-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15993
+          DAG.getConstant(0, dl, MVT::i32),
+          DAG.getConstant(AArch64CC::EQ, dl, MVT::i32), A.getValue(1));
+      return DAG.getMergeValues(
----------------
Can you make sure EQ is correct here? I think it might be backwards.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98264