[RISCV] Decouple Zve* extensions and the V extension.

[jacquesguan]

Summary

According to the spec, there are some difference between V and Zve64d. For example, the vmulh integer multiply variants that return the high word of the product (vmulh.vv, vmulh.vx, vmulhu.vv, vmulhu.vx, vmulhsu.vv, vmulhsu.vx) are not included for EEW=64 in Zve64*, but V extension does support these instructions. So I think that we could not mixed use Zve64d and the V extension.

[eopXD]

FeatureStdExtV will imply Zve32x, Zve64x, Zve32f and Zve64d, both RISCV.td or RISCVISAInfo does so.
So I wonder why do we need the extra condition here.
May you tell more?

[craig.topper]

The feature implication here is only used for llc .ll tests. Clang still thinks they are connected via the code in RISCVISAInfo.cpp

[jacquesguan]

In D117854#3260368, @eopXD wrote:

FeatureStdExtV will imply Zve32x, Zve64x, Zve32f and Zve64d, both RISCV.td or RISCVISAInfo does so.
So I wonder why do we need the extra condition here.
May you tell more?

For example, if I want to make vmulhsu.vv disable for Zve64d when EEW=64, I need some predicates or condition asserts to make sure that current target just implement only Zve64d but not the V extension.

[eopXD]

I see your point now.

In the v-spec describing for Zve:

All Zve* extensions support all vector integer instructions (Section Vector Integer Arithmetic Instructions), except that the vmulh integer multiply variants that return the high word of the product (vmulh.vv, vmulh.vx, vmulhu.vv, vmulhu.vx, vmulhsu.vv, vmulhsu.vx) are not included for EEW=64 in Zve64*.

For V extension:

The V extension supports all vector integer instructions (Section Vector Integer Arithmetic Instructions).

So we need to have something to express that V is enabled.

---

If I have parsed correctly in the above, I think we should have something that indicates V ext. is specified rather than adding HasStdExtV to the existing predicates.

[jacquesguan]

In D117854#3260392, @eopXD wrote:

I see your point now.

In the v-spec describing for Zve:

All Zve* extensions support all vector integer instructions (Section Vector Integer Arithmetic Instructions), except that the vmulh integer multiply variants that return the high word of the product (vmulh.vv, vmulh.vx, vmulhu.vv, vmulhu.vx, vmulhsu.vv, vmulhsu.vx) are not included for EEW=64 in Zve64*.

For V extension:

The V extension supports all vector integer instructions (Section Vector Integer Arithmetic Instructions).

So we need to have something to express that V is enabled.

---

If I have parsed correctly in the above, I think we should have something that indicates V ext. is specified rather than adding HasStdExtV to the existing predicates.

In my opinion, FeatureStdExtV just means that we have the V extension. In the instruction level, the V extension includes all the Zve* extensions. But they still have different using space which V is for application processors and Zve* is for embedded processors. Maybe it's better to not mix these two things?

[eopXD]

In my opinion, FeatureStdExtV just means that we have the V extension. In the instruction level, the V extension includes all the Zve* extensions. But they still have different using space which V is for application processors and Zve* is for embedded processors. Maybe it's better to not mix these two things?

Yes I agree with you that they are different and shouldn't be mixed.
On the other hand, if we want to guard vmulh out of Zve* extension shouldn't we use FeatureStdExtV?

[jacquesguan]

In D117854#3260413, @eopXD wrote:

In my opinion, FeatureStdExtV just means that we have the V extension. In the instruction level, the V extension includes all the Zve* extensions. But they still have different using space which V is for application processors and Zve* is for embedded processors. Maybe it's better to not mix these two things?

Yes I agree with you that they are different and shouldn't be mixed.
On the other hand, if we want to guard vmulh out of Zve* extension shouldn't we use FeatureStdExtV?

Yes, this is why I remove FeatureStdExtZve64d from the implies of FeatureStdExtV, for normal v instructions, we have HasVInstructions* to handle it, and for the instructions that only enable for V extension, we could use FeatureStdExtV.

[eopXD]

Without removing the implication Zve64d we can still guard things using FeatureStdExtV right?
Zve64d does not imply V.

[eopXD]

Thanks for pointing out that Zve maybe shouldn’t be a “subset” of V.
The patch makes sense to me now.
I guess you will have to modify the implications under RISCVISAInfo.cpp too?

[eopXD]

I guess you will have to modify the implications under RISCVISAInfo.cpp too?

I was wrong. I think we don't need to modify any implications there.

[eopXD]

LGTM, I have created D117865 to fix the mis-guarded instructions .
Thank you for the patch and explaination!

[craig.topper]

If we don’t change the implications in RISCVISAInfo then clang will still always enable Zve when V is in the march string. Is that what we want?

[eopXD]

A question that bothers me is that we need to specify a specific feature to enable the TARGET_BUILTIN for rvv, if we are removing the “subset” relationship here, how can V enable the builtin-s?

[craig.topper]

In D117854#3261618, @eopXD wrote:

A question that bothers me is that we need to specify a specific feature to enable the TARGET_BUILTIN for rvv, if we are removing the “subset” relationship here, how can V enable the builtin-s?

If I remember right "experimental-zve32x|experimental-v" should be valid syntax. Though I'm not sure the string is used at all because of how the clang_builtin_alias used by the header works.

[eopXD]

In D117854#3261659, @craig.topper wrote:

In D117854#3261618, @eopXD wrote:

A question that bothers me is that we need to specify a specific feature to enable the TARGET_BUILTIN for rvv, if we are removing the “subset” relationship here, how can V enable the builtin-s?

If I remember right "experimental-zve32x|experimental-v" should be valid syntax. Though I'm not sure the string is used at all because of how the clang_builtin_alias used by the header works.

I see. I will change my patch and see what I should do to address your comments.
Thank you.

[jacquesguan]

Address comment.

[jacquesguan]

In D117854#3261659, @craig.topper wrote:

In D117854#3261618, @eopXD wrote:

A question that bothers me is that we need to specify a specific feature to enable the TARGET_BUILTIN for rvv, if we are removing the “subset” relationship here, how can V enable the builtin-s?

If I remember right "experimental-zve32x|experimental-v" should be valid syntax. Though I'm not sure the string is used at all because of how the clang_builtin_alias used by the header works.

I modified the RISCVISAInfo and make this revision works for Clang.

[jacquesguan]

Fix test cases.

[eopXD]

Maybe change the title to address more specifically of what this patch does?

[jacquesguan]

In D117854#3263206, @eopXD wrote:

Maybe change the title to address more specifically of what this patch does?

Done, thanks.

[craig.topper]

clang/lib/Sema/SemaChecking.cpp

3978 ↗

(On Diff #402173)

Is this change testable?

llvm/lib/Support/RISCVISAInfo.cpp

697 ↗

(On Diff #402173)

Should we check that Zve and V are not specified together?

[jacquesguan]

Address comment

[jacquesguan]

clang/lib/Sema/SemaChecking.cpp

3978 ↗

(On Diff #402173)

Done, I add a test case clang/test/CodeGen/RISCV/rvv-intrinsics/rvv-error.c to test this.

llvm/lib/Support/RISCVISAInfo.cpp

697 ↗

(On Diff #402173)

Done.

[jacquesguan]

Fix test case

[craig.topper]

llvm/lib/Target/RISCV/RISCVSubtarget.h

185

This should be (HasStdExtV || HasStdExtZve32f) && HasStdExtZfh. V does mean f16 vectors are enabled.

[jacquesguan]

Address comment

[craig.topper]

LGTM

[jacquesguan]

Rebase main

llvm/lib/Target/RISCV/RISCV.td

def FeatureStdExtZve64d
     : SubtargetFeature<"experimental-zve64d", "HasStdExtZve64d", "true",
                        "'Zve64d' (Vector Extensions for Embedded Processors "
                        "with maximal 64 EEW, F and D extension)",
                        [FeatureStdExtZve64f]>;
 
 def FeatureStdExtV
     : SubtargetFeature<"experimental-v", "HasStdExtV", "true",
                        "'V' (Vector Extension for Application Processors)",
-                       [FeatureStdExtZvl128b, FeatureStdExtZve64d, FeatureStdExtF, FeatureStdExtD]>;
+                       [FeatureStdExtZvl128b, FeatureStdExtF, FeatureStdExtD]>;
 
 def HasVInstructions    : Predicate<"Subtarget->hasVInstructions()">,
       AssemblerPredicate<
-          (any_of FeatureStdExtZve32x),
+          (any_of FeatureStdExtZve32x, FeatureStdExtV),
           "'V' (Vector Extension for Application Processors), 'Zve32x' or "
           "'Zve64x' (Vector Extensions for Embedded Processors)">;
 def HasVInstructionsI64 : Predicate<"Subtarget->hasVInstructionsI64()">,
       AssemblerPredicate<
-          (any_of FeatureStdExtZve64x),
+          (any_of FeatureStdExtZve64x, FeatureStdExtV),
           "'V' (Vector Extension for Application Processors) or 'Zve64x' "
           "(Vector Extensions for Embedded Processors)">;
 def HasVInstructionsAnyF : Predicate<"Subtarget->hasVInstructionsAnyF()">,
       AssemblerPredicate<
-          (any_of FeatureStdExtZve32f),
+          (any_of FeatureStdExtZve32f, FeatureStdExtV),
           "'V' (Vector Extension for Application Processors), 'Zve32f', "
           "'Zve64f' or 'Zve64d' (Vector Extensions for Embedded Processors)">;
 
 def Feature64Bit
     : SubtargetFeature<"64bit", "HasRV64", "true", "Implements RV64">;
 def IsRV64 : Predicate<"Subtarget->is64Bit()">,
                        AssemblerPredicate<(all_of Feature64Bit),
                        "RV64I Base Instruction Set">;

llvm/lib/Target/RISCV/RISCVSubtarget.h

public:
   }
   RISCVABI::ABI getTargetABI() const { return TargetABI; }
   bool isRegisterReservedByUser(Register i) const {
     assert(i < RISCV::NUM_TARGET_REGS && "Register out of range");
     return UserReservedRegister[i];
   }
 
   // Vector codegen related methods.
-  bool hasVInstructions() const { return HasStdExtZve32x; }
-  bool hasVInstructionsI64() const { return HasStdExtZve64x; }
-  bool hasVInstructionsF16() const { return HasStdExtZve32f && HasStdExtZfh; }
+  bool hasVInstructions() const { return HasStdExtV || HasStdExtZve32x; }
+  bool hasVInstructionsI64() const { return HasStdExtV || HasStdExtZve64x; }
+  bool hasVInstructionsF16() const {
+    return HasStdExtV || (HasStdExtZve32f && HasStdExtZfh);
+  }
   // FIXME: Consider Zfinx in the future
-  bool hasVInstructionsF32() const { return HasStdExtZve32f && HasStdExtF; }
+  bool hasVInstructionsF32() const {
+    return HasStdExtV || (HasStdExtZve32f && HasStdExtF);
+  }
   // FIXME: Consider Zdinx in the future
-  bool hasVInstructionsF64() const { return HasStdExtZve64d && HasStdExtD; }
+  bool hasVInstructionsF64() const {
+    return HasStdExtV || (HasStdExtZve64d && HasStdExtD);
+  }
   // F16 and F64 both require F32.
   bool hasVInstructionsAnyF() const { return hasVInstructionsF32(); }
   unsigned getMaxInterleaveFactor() const {
     return hasVInstructions() ? MaxInterleaveFactor : 1;
   }
 
 protected:
   // GlobalISel related APIs.