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

[eopXD]

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.

[eopXD]

Change:

• Address commments on restrictions in RISCVSubtarget.cpp
• Fix clang-format fail
• Fix test case fail

[eopXD]

llvm/lib/Target/RISCV/RISCVSubtarget.cpp

181

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

[eopXD]

Update code.

[eopXD]

Update code.

llvm/lib/Target/RISCV/RISCVSubtarget.cpp

144–152

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

[eopXD]

Fix test case.

[eopXD]

Rebase to newest main.

[eopXD]

Rebase.

[eopXD]

Rebase.

[eopXD]

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

[craig.topper]

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]

llvm/lib/Support/RISCVISAInfo.cpp

800

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

[eopXD]

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]

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]

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]

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]

Rebase to latest main.

[craig.topper]

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]

Address comments from Craig.

[eopXD]

Ping, thank you.

[achieveartificialintelligence]

llvm/lib/Support/RISCVISAInfo.cpp

737–747

Can we compress the code?

llvm/lib/Target/RISCV/RISCV.td

153–186

Same as here.

[eopXD]

Rebase.

[eopXD]

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]

ping again, thank you.

[achieveartificialintelligence]

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]

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]

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.

[craig.topper]

LGTM

[eopXD]

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

[eopXD]

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