This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add the zvl extension according to the v1.0 spec
ClosedPublic

Authored by eopXD on Aug 25 2021, 4:36 AM.

Details

Summary

zvl is the new standard vector extension that specifies the minimum vector length of the vector extension.
The zvl extension is related to the zve extension and other updates that are added in v1.0.

According to https://github.com/riscv-non-isa/riscv-c-api-doc/pull/21,
Clang defines macro __riscv_v_min_vlen for zvl and it can be used for applications that uses the vector extension.
LLVM checks whether the option riscv-v-vector-bits-min (if specified) matches the zvl* extension specified.

Diff Detail

Event Timeline

eopXD created this revision.Aug 25 2021, 4:36 AM
eopXD requested review of this revision.Aug 25 2021, 4:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2021, 4:36 AM
eopXD updated this revision to Diff 368608.Aug 25 2021, 4:45 AM

Update code, bug fixes to this patch.

eopXD added inline comments.Aug 25 2021, 4:49 AM
llvm/lib/Target/RISCV/RISCV.td
211

Don't need the AssemblerPredicate here because this sub-extension is used to restrict vlen, which is treated as a constant in the assembly.

llvm/lib/Target/RISCV/RISCVSubtarget.h
137

I don't think this function will be called. However other sub-extensions will have this function so I still created it.

kito-cheng added inline comments.Aug 25 2021, 5:36 AM
llvm/lib/Support/RISCVISAInfo.cpp
72

Although the table in vector spec only list zvl32b~zvl1024b, but there is note say Longer vector length extensions should follow the same pattern.[1], so I would suggest let enumerate to 65536, the unbound of VLEN[2],

[1] https://github.com/riscv/riscv-v-spec/blob/master/v-spec.adoc#181-zvl-minimum-vector-length-standard-extensions
[2] https://github.com/riscv/riscv-v-spec/blob/master/v-spec.adoc#2-implementation-defined-constant-parameters

llvm/lib/Target/RISCV/RISCVSubtarget.cpp
139

I guess this should be more than an assertion? but I am not sure does it make sense to emit error or warning here? or just silently return ZvlLen if RVVVectorBitsMin is less than ZvlLen.

kito-cheng added inline comments.Aug 25 2021, 7:02 PM
llvm/lib/Support/RISCVISAInfo.cpp
72

Oh, typo, unbound should be upper bound :p

eopXD updated this revision to Diff 368796.Aug 25 2021, 8:03 PM
eopXD marked 2 inline comments as done.

Address comments.

  • enumerate zvl up to 65536
  • fix grammitcal error in assertion error message
llvm/lib/Target/RISCV/RISCVSubtarget.cpp
139

I think the compiler should not return ZvlLen silently if the specified RVVVectorBitsMin is less. As zvl hard restricts the minimum vlen, the user should be aware (notified) of that. I think the assertion error here can guide the user to either increase their RVVVectorBitsMin in the argument or compile under another architecture specification.

craig.topper added inline comments.
llvm/lib/Target/RISCV/RISCVSubtarget.cpp
117

The max should be greater than ZvlLen right?

132

Some of the callers of this will break if it returns a value less than 64. We need to limit ELEN to 32 if VLEN is 32. And we can't use LMUL=1/8 with i8 vectors, or LMUL=1/4 with i16 vector, or LMUL=1/2 with i32 vector.

139

assertions are compiled out of release builds. The code after the assertions was trying to do something sane for release builds.

I think we need to visit some larger aspects of our vector implementation. Here are some thoughts.

-Most uses of Subtarget.hasStdExtV() don't really mean what the spec calls the standard V extension. They just means that we have vector instructions. Could be V, could be one of the Zve32* or Zve64* extensions.
-V extension passed to -march should imply at least Zvl128b.
-V extension passed to -march should enable F and D.
-Does Zvl32b passed to march enable vector instructions? Or do we still need Zve32* or Zve64* or V?
-If Zvl32b is in effect the i64 and f64 RVV intrinsics need to be disabled.

Those rule are unclear to me too, I created an issue on vector spec, I guess we need writing few special rule for those extensions...

https://github.com/riscv/riscv-v-spec/issues/723

I think we need to visit some larger aspects of our vector implementation. Here are some thoughts.

-Most uses of Subtarget.hasStdExtV() don't really mean what the spec calls the standard V extension. They just means that we have vector instructions. Could be V, could be one of the Zve32* or Zve64* extensions.
-V extension passed to -march should imply at least Zvl128b.
-V extension passed to -march should enable F and D.
-Does Zvl32b passed to march enable vector instructions? Or do we still need Zve32* or Zve64* or V?
-If Zvl32b is in effect the i64 and f64 RVV intrinsics need to be disabled.

HsiangKai added inline comments.Aug 26 2021, 12:42 AM
llvm/lib/Target/RISCV/RISCVSubtarget.cpp
132

We have an implicit assumption that VLEN >= 64 from our VLA types. I suggest use 64 as the lower bound and add a FIXME here for VLEN is 32.

139

Should it be RVVVectorBitsMin < ZvlLen?

I suggest to return 0 and print a warning message for users.

HsiangKai added inline comments.Aug 26 2021, 12:55 AM
llvm/lib/Target/RISCV/RISCVSubtarget.cpp
117

Should it be less than or equal to ZvlLen? I would suggest to return 0 when RVVVectorBitsMax > ZvlLen.
The value also needs to >= 64.

craig.topper added inline comments.Aug 26 2021, 1:08 AM
llvm/lib/Target/RISCV/RISCVSubtarget.cpp
139

This isn't a good place to print warnings. It would just be a random message printed to stderr without going through any of clang's diagnostic infrastructure.

I think we need to visit some larger aspects of our vector implementation. Here are some thoughts.

-Most uses of Subtarget.hasStdExtV() don't really mean what the spec calls the standard V extension. They just means that we have vector instructions. Could be V, could be one of the Zve32* or Zve64* extensions.
-V extension passed to -march should imply at least Zvl128b.
-V extension passed to -march should enable F and D.
-Does Zvl32b passed to march enable vector instructions? Or do we still need Zve32* or Zve64* or V?
-If Zvl32b is in effect the i64 and f64 RVV intrinsics need to be disabled.

We need to apply the constraints of Zve* to intrinsics/pseudo instructions. I am not sure what is the behavior if users specify V and Zve* at the same time? I think Zve* is more complicated. We could prepare another patch for it.

Could we support Zvl32b? We already have an implicit constraint of vlen >= 64 from our VLA types, right?

eopXD added inline comments.Aug 26 2021, 1:11 AM
llvm/lib/Target/RISCV/RISCVSubtarget.cpp
139

I think moving the check to initializeSubtargetDependencies and use report_fatal_error will be a more appropriate approach?

craig.topper added inline comments.Aug 26 2021, 1:13 AM
llvm/lib/Target/RISCV/RISCVSubtarget.cpp
139

I think that causes clang to generate a crash report and telling the user to file a bug.

Forgot to mention, all this code is also being modified by D107290

eopXD added inline comments.Aug 26 2021, 1:20 AM
llvm/lib/Target/RISCV/RISCVSubtarget.cpp
139

I see, then my proposal is invalid.

I am not yet that familiar with the code base. I see RISCVFrameLowering.cpp using LLVMContext::diagnose, but RISCVSubtarget here doesn't seem to have access to LLVMContext.

May you recommend a pointer to some diagnose function I can use to tell the user this is an invalid setting between RVVVectorBitsMin(from riscv-v-vector-bits-min) and ZvlLen (from -march=zvl*b)?

I also don't think we're ready/able to support Zvl32b for the reasons stated above. I think the smoothest path forward would be, as @HsiangKai suggests, to only support VLEN>=64 and revisit 32. Supporting 32 is (I think) a much larger thing owing to our codegen assumptions on VLEN and ELEN. And, as @craig.topper said, there are a few little things that would probably be best prepared before this patch, replacing some of the calls to hasStdExtV at least (hasVVectors, for codegen purposes?). It sounds like we're unsure on some of the finer details about how the extensions interact.

I also don't think we're ready/able to support Zvl32b for the reasons stated above. I think the smoothest path forward would be, as @HsiangKai suggests, to only support VLEN>=64 and revisit 32. Supporting 32 is (I think) a much larger thing owing to our codegen assumptions on VLEN and ELEN. And, as @craig.topper said, there are a few little things that would probably be best prepared before this patch, replacing some of the calls to hasStdExtV at least (hasVVectors, for codegen purposes?). It sounds like we're unsure on some of the finer details about how the extensions interact.

I think our mapping from lmul/sew to <vscale x Y x iZ> types also break with VLEN=32. I haven't been able to figure out a single mapping that works for VLEN=32 and VLEN=64 and supports i64 vectors. To support VLEN=32 we need to reduce RVVBitsPerBlock to 32, but then you can't fit an i64 element into it.

khchen added a subscriber: khchen.Sep 1 2021, 12:47 AM
eopXD updated this revision to Diff 381737.Oct 23 2021, 6:37 AM

Address comments and add macro in clang.

eopXD retitled this revision from [RISCV] Add the zvl extension according to the v1.0-rc1 spec to [WIP][RISCV] Add the zvl extension according to the v1.0-rc2 spec.Oct 23 2021, 8:46 AM
eopXD edited the summary of this revision. (Show Details)
eopXD updated this revision to Diff 381747.Oct 23 2021, 9:13 AM

Rebase.

eopXD updated this revision to Diff 382543.Oct 27 2021, 12:59 AM

Fix clang-format.

eopXD updated this revision to Diff 382544.Oct 27 2021, 1:01 AM

Update code.

eopXD edited the summary of this revision. (Show Details)Oct 27 2021, 1:11 AM
eopXD updated this revision to Diff 382571.Oct 27 2021, 2:31 AM

Add implication: v imply d and f

eopXD updated this revision to Diff 382574.Oct 27 2021, 2:40 AM

Address comments:

  • remove duplicate checkDependency()
  • stay consistent in my patch of using auto for StringRef
eopXD updated this revision to Diff 382582.Oct 27 2021, 2:56 AM

Update.

eopXD updated this revision to Diff 382628.Oct 27 2021, 5:50 AM
eopXD marked 4 inline comments as done.Oct 27 2021, 5:50 AM

Change:

  • Address commments on restrictions in RISCVSubtarget.cpp
  • Fix clang-format fail
  • Fix test case fail
eopXD added inline comments.Oct 27 2021, 5:53 AM
llvm/lib/Target/RISCV/RISCVSubtarget.cpp
139

I see code under RISCVSubtarget::initializeSubtargetDependencies also using report_fatal_error:

eopXD marked an inline comment as not done.Oct 27 2021, 6:01 AM
eopXD updated this revision to Diff 382879.Oct 27 2021, 6:45 PM

Update code.

eopXD updated this revision to Diff 382880.Oct 27 2021, 6:48 PM

Update code.

llvm/lib/Target/RISCV/RISCVSubtarget.cpp
117

@HsiangKai I think you mis-typed the equation? It should be RVVVectorBitsMax < ZvlLen ;)

eopXD updated this revision to Diff 383599.Oct 30 2021, 9:48 AM

Fix test case.

eopXD updated this revision to Diff 383601.Oct 30 2021, 10:12 AM

Rebase to newest main.

eopXD updated this revision to Diff 383952.Nov 1 2021, 7:59 PM

Rebase.

eopXD retitled this revision from [WIP][RISCV] Add the zvl extension according to the v1.0-rc2 spec to [RISCV] Add the zvl extension according to the v1.0-rc2 spec.Nov 2 2021, 12:09 AM
eopXD edited the summary of this revision. (Show Details)
eopXD retitled this revision from [RISCV] Add the zvl extension according to the v1.0-rc2 spec to [RISCV] Add the zvl extension according to the v1.0 spec.Nov 2 2021, 1:29 AM
eopXD edited the summary of this revision. (Show Details)
eopXD updated this revision to Diff 393185.Dec 9 2021, 8:51 AM

Rebase now since the preceeding patches are accepted.
This patch is ready for review.

craig.topper added inline comments.Dec 13 2021, 12:14 PM
llvm/lib/Support/RISCVISAInfo.cpp
724

I think I'd like to see this as a static data structure rather than building a StringMap on the fly.

Maybe like

static const char *zvl64bimplied[] = { "zvl32b" };
static const char *zvl128bimplied[] = { "zvl64b" };
...

struct ImpliedEntry = {
  StringLiteral Name;
  ArrayRef<const char*> ImpliedExtensions;
};

static constexpr ImpliedEntry ImpliedTable[] = {
  { "zvl64b", zvl64bimplied },
  { "zvl128b", zvl128implied },
  ...
};

You can then use std::lower_bound to search the ImpliedTable to find the correct row of ImpliedTable. I haven't tested this. Maybe I'll put up a patch on the existing V implications as a proof of concept.

781

I think we should check the return value from consume_back and getAsInteger to make sure we really parsed what we think we parsed. That will prevent surprises if a new extension comes along that also starts with "zvl"

llvm/lib/Target/RISCV/RISCVSubtarget.cpp
132

This needs the same FIXME as above.

craig.topper added inline comments.Dec 13 2021, 6:22 PM
llvm/lib/Support/RISCVISAInfo.cpp
781

This would match for "zvlsseg" right now wouldn't it?

eopXD added inline comments.Dec 13 2021, 6:25 PM
llvm/lib/Support/RISCVISAInfo.cpp
781

This would match for "zvlsseg" right now wouldn't it?

Yes, you have a point.

IsZvlExt should be ExtName.consume_front("zvl") && ExtName.consume_back("b").

eopXD updated this revision to Diff 394119.Dec 13 2021, 7:11 PM
eopXD marked 8 inline comments as done.

Rebase and address comment.

llvm/lib/Target/RISCV/RISCVSubtarget.cpp
139

I am marking this thread as done since there are no further comments.

This patch currently uses report_fatal_error for it. Although as Craig mentioned it may not be ideal.

frasercrmck added inline comments.Dec 14 2021, 6:47 AM
clang/test/Preprocessor/riscv-target-features.c
230

Are we able to test non-default values of __riscv_v_min_vlen here?

llvm/lib/Target/RISCV/RISCVSubtarget.cpp
118

Is this intuitive behaviour? If the user supplies RVVVectorBitsMax and it's less than ZvlLen, should it silently return? Or do we instead see RVVVectorBitsMax as a user-guided limit on top of the architecture? Which means it can be less but not more? I'm not sure.

eopXD updated this revision to Diff 394454.Dec 14 2021, 7:06 PM
eopXD marked 3 inline comments as done.

Rebase and address comments.

clang/test/Preprocessor/riscv-target-features.c
230

Yes we shall, thank you for the reminder.

llvm/lib/Target/RISCV/RISCVSubtarget.cpp
118

Added report_fatal_error here.

eopXD updated this revision to Diff 394471.Dec 14 2021, 10:56 PM

Rebase to latest main.

craig.topper added inline comments.Dec 15 2021, 5:30 PM
llvm/lib/Support/RISCVISAInfo.cpp
722–733

Need curly braces around the strings too.

782

getAsInteger returns an indication of success or failure. We should check that too. That will protect us in case someone creates an extension like "zvlfoob" in the future.

eopXD updated this revision to Diff 394739.Dec 15 2021, 9:17 PM
eopXD marked 2 inline comments as done.

Address comments from Craig.

Ping, thank you.

llvm/lib/Support/RISCVISAInfo.cpp
723–733

Can we compress the code?

llvm/lib/Target/RISCV/RISCV.td
145–178

Same as here.

eopXD updated this revision to Diff 396505.Dec 29 2021, 1:26 AM

Rebase.

eopXD added inline comments.Dec 29 2021, 1:35 AM
llvm/lib/Support/RISCVISAInfo.cpp
723–733

Hi,

Thank you for leaving a comment. Do you mean to embed the list of implied extensions into the declaration of ImpliedExts? Like:

static constexpr ImpliedExtsEntry ImpliedExts[] = {
    {{"v"}, {{"zvlsseg", "zvl128b"}}},
   ...

In my opinion I think compressing here may not help because the indirection is intended to have the implications be in more sorted order that is more human readable since ImpliedExts are required to be in lexicographical order.

I am just stating my opinion on my current implementation since I think this is a coding style problem. What do you think?

eopXD added a comment.Jan 5 2022, 9:14 PM

ping again, thank you.

llvm/lib/Target/RISCV/RISCV.td
147–179

I think we can write in this way:

foreach i = { 5-15 } in {
  defvar I = !shl(2, i);
  def FeatureStdExtZvl#I#b
      : SubtargetFeature<"experimental-zvl"#I#"b", "ZvlLen", "ExtZvl::Zvl"#I#"b",
                        "'Zvl' (Minimum Vector Length) "#I,
                        [!cast<SubtargetFeature>("FeatureStdExtZvl"#!srl(I, 1)#"b")]>;
}
craig.topper added inline comments.Jan 11 2022, 8:50 PM
llvm/lib/Support/RISCVISAInfo.cpp
751

Use StringRef instead of auto here.

752

You can keep this auto though. iterators are ugly.

llvm/lib/Target/RISCV/RISCV.td
180

Is this used?

llvm/lib/Target/RISCV/RISCVSubtarget.cpp
121

Why do we need an explicit Twine construction here?

132

Same here.

eopXD updated this revision to Diff 400098.Jan 14 2022, 11:58 AM
eopXD marked 8 inline comments as done.

Address comments.
Thanks for reviewing!

llvm/lib/Target/RISCV/RISCV.td
147–179

Thank you for the tip! I previously don't know TableGen can compute values like bit-shift left and do value type casting.

Adapted your code snippet. I modified a bit because Zvl32b don't imply anything.

180

No it is not, deleted.

This revision is now accepted and ready to land.Jan 14 2022, 12:02 PM
eopXD updated this revision to Diff 400111.Jan 14 2022, 12:47 PM

Rebase to latest main.
Resolve test case conflicts due to Zvamo removal.

eopXD updated this revision to Diff 400238.EditedJan 14 2022, 9:56 PM

Rebase to latest main.
Resolve test case conflicts due to Zfh, Zfhmin version update.

This revision was landed with ongoing or failed builds.Jan 14 2022, 11:01 PM
This revision was automatically updated to reflect the committed changes.