[PATCH] D143570: [RISCV][MC] Add support for RV64E

Wed Feb 8 22:09:53 PST 2023

pcwang-thead added inline comments.

================
Comment at: llvm/lib/Target/RISCV/RISCVFeatures.td:499
+                        AssemblerPredicate<(all_of FeatureRVE)>;

 def FeatureRelax
----------------
jrtc27 wrote:
> pcwang-thead wrote:
> > pcwang-thead wrote:
> > > How about this?
> > > ```
> > > def IsRVE : Predicate<"Subtarget->isRVE()">,
> > >                         AssemblerPredicate<(all_of FeatureRVE)>;
> > > def IsRV32E : Predicate<"Subtarget->is32RVE()">,
> > >                         AssemblerPredicate<(all_of FeatureRVE, Feature32Bit)>;
> > > def IsRV64E : Predicate<"Subtarget->is64RVE()">,
> > >                        AssemblerPredicate<(all_of FeatureRVE, Feature64Bit)>;
> > > ```
> > Oops, I mean `Subtarget->isRV32E()` and `Subtarget->isRV64E()`.
> Why do you need that? Just use IsRV32/64 and IsRVE as appropriate. We don't have IsRV32F, IsRV64D, IsRV32A, etc.
I have no strong opinion on this, it's just a suggestion on one of the concerns in the patch description:
> the existing feature (FeatureRV32E) and predicate (IsRV32E) have been renamed to FeatureRVE and IsRVE. The acronym "RVE" might not be immediately clear to people;

And we do have `RV32E` and `RV64E` in the spec, maybe we should rename them to `RVE`?

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

https://reviews.llvm.org/D143570