[PATCH] D112408: [WIP][RISCV] Add the zve extension according to the v1.0-rc2 spec

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:182
 
+def FeatureExtZve32x
+    : SubtargetFeature<"experimental-zve32x", "HasStdExtZve32x", "true",
----------------
craig.topper wrote:
> frasercrmck wrote:
> > Do we need to define distinct `SubtargetFeature`s for each of these extensions or could they be broken down into a single `MaxEEW` feature (32 or 64) in conjunction with the pre-existing F/D features. This seems like it's more complicated than it needs to be.
> I don't think it is quite that simple. Couldn't you have a scalar D and have zve64f vector?
Yes, that's fair. Though I still think we can create a more intuitive system of `Predicate`s to handle the TableGen aspects, as you've begun to do in D112496.


================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.h:141
+  // either v or zve* suppaort v instructions
+  bool hasStdExtV() const { return HasStdExtV || HasStdExtZve32x; }
+  bool hasStdExtZve32x() const { return HasStdExtZve32x; }
----------------
craig.topper wrote:
> frasercrmck wrote:
> > Is this correct? I thought we'd keep `hasStdExtV` as being the single-letter V extension, and Zve32x isn't that.
> I just put up D112496 to stop using hasStdExtV everywhere.
Ah right now I see what this was trying to do. I think your patch helps things, thanks.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/fixed-vectors-bitcast.ll:158
 ; CHECK-NEXT:    vmv.x.s a0, v8
+; CHECK-NEXT:    lui a1, 1048560
+; CHECK-NEXT:    or a0, a0, a1
----------------
craig.topper wrote:
> frasercrmck wrote:
> > What's going on here, do you know?
> I believe this is coming from the nan-boxing for f16 in RISCVTargetLowering::splitValueIntoRegisterParts. The addition of +f must have changed PartVT from i32/i64 to f32. Even though we're using i32 for the return due to ABI.
Ah, interesting. I can't tell if that's a bug fix, i.e., if it's invalid to compile this test without `f` - though shouldn't we pass `experimental-zfh` by that same logic? Regardless, maybe we could split this off and pre-commit it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112408