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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
