Page MenuHomePhabricator

[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

There are a very large number of changes, so older changes are hidden. Show Older Changes
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
181

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
144–152

@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
751

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.

800

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
174

This needs the same FIXME as above.

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

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
800

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
181

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
250

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

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

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
250

Yes we shall, thank you for the reminder.

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

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
736–747

Need curly braces around the strings too.

801

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
737–747

Can we compress the code?

llvm/lib/Target/RISCV/RISCV.td
153–186

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
737–747

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
155–187

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
769

Use StringRef instead of auto here.

770

You can keep this auto though. iterators are ugly.

llvm/lib/Target/RISCV/RISCV.td
188

Is this used?

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

Why do we need an explicit Twine construction here?

167

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
155–187

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.

188

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.