Page MenuHomePhabricator

[RISCV] Unify the arch string parsing logic to RISCVISAInfo.
Needs ReviewPublic

Authored by kito-cheng on Jun 29 2021, 8:26 PM.

Details

Summary

How many place you need to modify when implementing a new extension for RISC-V?

At least 7 places as I know:

  • Add new SubtargetFeature at RISCV.td
  • -march parser in RISCV.cpp
  • RISCVTargetInfo::initFeatureMap@RISCV.cpp for handling feature vector.
  • RISCVTargetInfo::getTargetDefines@RISCV.cpp for pre-define marco.
  • Arch string parser for ELF attribute in RISCVAsmParser.cpp
  • ELF attribute emittion in RISCVAsmParser.cpp, and make sure it's in canonical order...
  • ELF attribute emittion in RISCVTargetStreamer.cpp, and again, must in canonical order...

And now, this patch provide an unified infrastructure for handling (almost)
everything of RISC-V arch string.

After this patch, you only need to update 2 places for implement an extension
for RISC-V:

  • Add new SubtargetFeature at RISCV.td, hmmm, it's hard to avoid.
  • Add new entry to SupportedExtensionInfos@RISCVISAInfo.cpp.

Most codes are come from existing -march parser, but with few new feature/bug
fixes:

  • Accept version for -march, e.g. -march=rv32i2p0.
  • Reject version info with p but without minor version number like rv32i2p.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Changes:

  • New function RISCVISAInfo::isSupportedExtensionFeature
  • Remove parameter CheckExperimental from RISCVISAInfo::isSupportedExtension
  • Clean up obvious comments
  • Address @jrtc27's and @craig.topper's comments
kito-cheng marked 8 inline comments as done.Jul 23 2021, 2:29 AM
kito-cheng added inline comments.
clang/test/Driver/riscv-arch.c
198–201

We support E-extension on MC-layer...but not support ilp32e.

This patch unify the ISA stuffs, so either I need to remove that from MC, or I need to made rv32e supported on driver...

https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp#L50
https://github.com/llvm/llvm-project/blob/main/llvm/test/MC/RISCV/rv32e-valid.s
https://github.com/llvm/llvm-project/blob/main/llvm/test/MC/RISCV/rv32e-invalid.s

llvm/lib/Support/RISCVISAInfo.cpp
41

Yeah, it's actually just an array of chars, but we have use find function from StringRef :p

136

Good point, CheckExperimental is kind of ambiguous, new function: isSupportedExtensionFeature added.

kito-cheng marked an inline comment as done.

Changes:

  • Rename RISCVISAInfo::parse to RISCVISAInfo::parseFeatures/RISCVISAInfo::parseArchString
kito-cheng marked an inline comment as done.Jul 23 2021, 2:43 AM
kito-cheng added inline comments.
llvm/include/llvm/Support/RISCVISAInfo.h
46

Rename RISCVISAInfo::parse to RISCVISAInfo::parseFeatures/RISCVISAInfo::parseArchString, I guess it should be more clear.

frasercrmck added inline comments.Jul 26 2021, 4:32 AM
llvm/lib/Support/RISCVISAInfo.cpp
128

This looks like a find_if if that'd make it any simpler.

148

This also looks like a find_if

232

Not sure why these need to be declared up here.

311

Failed to parse ...

316

Failed to parse

360

Commented-out code

415

I think you can just have if (llvm::any_of(Arch, isupper)) here.

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2066

Don't need {} here

2085

Don't need {} here either

kito-cheng marked an inline comment as done.

Changes:

kito-cheng marked 14 inline comments as done.Jul 26 2021, 11:31 PM
jrtc27 added inline comments.Jul 27 2021, 7:05 AM
llvm/include/llvm/Support/RISCVISAInfo.h
32

Does Exts need initialising to be empty here? I can never remember

48–53

I wonder, should these be constructors instead? RISCVISAInfo ISAInfo; ISAInfo.parse doesn't feel very idiomatic C++

llvm/lib/Support/RISCVISAInfo.cpp
41

Still not a StringLiteral

44

I do think it would be better to not list Experimental for the non-experimental ones

179

Why not just return these directly? Don't need a variable.

195

I imagine this needs to change to the new Sm-/Ss- scheme, but I don't know which way round those are intended to go

198

Do we know if this is still the case? Ss- is being used for S-mode extensions and Sm- for M-mode extensions, so I'd expect Sh- (or maybe just Ss-) for HS-mode extensions, not H-.

208

llvm_unreachable

215

Why not put this in its switch case?

226–230

Don't know if this is better or not, but this is the more compact way to write it

232

Yeah, declare the variables at their assignments. Besides, you don't even need variables for the single-letter case, just compare the return values of the functions directly.

kito-cheng updated this revision to Diff 364758.Aug 6 2021, 5:26 AM
kito-cheng marked an inline comment as done.

Changes:

  • Forbid copy ctor and operator= for RISCVISAInfo.
  • Move RISCVISAInfo's constructor to private.
  • Refine RISCVISAInfo::parse* and made they become static function.
  • Address @jrtc27's comment.
kito-cheng added inline comments.Aug 6 2021, 5:34 AM
llvm/include/llvm/Support/RISCVISAInfo.h
32

std::map has default construct that will construct an empty map.

llvm/lib/Support/RISCVISAInfo.cpp
195

I would prefer submit another patch to make this parser up to date, and keep this patch as a refactor patch as possible:

e.g.:

  • Full implication info, e.g. d implied f, f implied zicsr...
  • Prefix zxm.
198

Like above reply, prefer another patch to fix that.

226–230

It's more compact but it's hard to read the rule to me, so I would prefer keep that as it is

jrtc27 added inline comments.Aug 6 2021, 5:39 AM
llvm/include/llvm/Support/RISCVISAInfo.h
50–51

Use llvm::Expected<...> as the return value to avoid a separate error out param

llvm/lib/Support/RISCVISAInfo.cpp
41

Is the "Infos" suffix on the variable name really needed?

42

I'd keep these all one per line

108
379

No error?..

kito-cheng added inline comments.Aug 12 2021, 2:18 AM
llvm/lib/Support/RISCVISAInfo.cpp
42

Although it's format by clang-format, but I like one per line too, let me manually update that.

108

It's format by clang-format, but either is fine to me :P

379

Not all features is related to extension, like relax or save-restore, so here we just ignore, but add few comment here seem better.

Changes:

  • Rename class, strip the Info:
    • RISCVSupportedExtensionInfo -> RISCVSupportedExtension
  • Rename variables:
    • SupportedExtensionInfos -> SupportedExtensions
    • SupportedExperimentalExtensionInfos -> SupportedExperimentalExtensions
  • Change return type of parseArchString to llvm::Expected<std::unique_ptr<RISCVISAInfo>>
  • Address @jrtc27's comment.
kito-cheng edited the summary of this revision. (Show Details)Aug 18 2021, 8:06 PM
asb added a comment.Aug 19 2021, 5:42 AM

I'm getting a build error (building with clang 12.0.1):

FAILED: lib/Support/CMakeFiles/LLVMSupport.dir/RISCVISAInfo.cpp.o 
/usr/bin/clang++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/home/asb/llvm-project/build/default/lib/Support -I/home/asb/llvm-project/llvm/lib/Support -I/home/asb/llvm-project/build/default/include -I/home/asb/llvm-project/llvm/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -Wmisleading-indentation -fdiagnostics-color -Werror=global-constructors -g -fPIC -gsplit-dwarf -std=c++14  -fno-exceptions -fno-rtti -MD -MT lib/Support/CMakeFiles/LLVMSupport.dir/RISCVISAInfo.cpp.o -MF lib/Support/CMakeFiles/LLVMSupport.dir/RISCVISAInfo.cpp.o.d -o lib/Support/CMakeFiles/LLVMSupport.dir/RISCVISAInfo.cpp.o -c /home/asb/llvm-project/llvm/lib/Support/RISCVISAInfo.cpp
/home/asb/llvm-project/llvm/lib/Support/RISCVISAInfo.cpp:399:10: warning: moving a local object in a return statement prevents copy elision [-Wpessimizing-move]
  return std::move(ISAInfo);
         ^
/home/asb/llvm-project/llvm/lib/Support/RISCVISAInfo.cpp:399:10: note: remove std::move call here
  return std::move(ISAInfo);
         ^~~~~~~~~~       ~
/home/asb/llvm-project/llvm/lib/Support/RISCVISAInfo.cpp:478:12: error: call to deleted constructor of 'llvm::Error'
    return E;
           ^
/home/asb/llvm-project/llvm/include/llvm/Support/Error.h:186:3: note: 'Error' has been explicitly marked deleted here
  Error(const Error &Other) = delete;
  ^
/home/asb/llvm-project/llvm/include/llvm/Support/Error.h:493:18: note: passing argument to parameter 'Err' here
  Expected(Error Err)
                 ^
/home/asb/llvm-project/llvm/lib/Support/RISCVISAInfo.cpp:520:14: error: call to deleted constructor of 'llvm::Error'
      return E;
             ^
/home/asb/llvm-project/llvm/include/llvm/Support/Error.h:186:3: note: 'Error' has been explicitly marked deleted here
  Error(const Error &Other) = delete;
  ^
/home/asb/llvm-project/llvm/include/llvm/Support/Error.h:493:18: note: passing argument to parameter 'Err' here
  Expected(Error Err)
                 ^
/home/asb/llvm-project/llvm/lib/Support/RISCVISAInfo.cpp:642:14: error: call to deleted constructor of 'llvm::Error'
      return E;
             ^
/home/asb/llvm-project/llvm/include/llvm/Support/Error.h:186:3: note: 'Error' has been explicitly marked deleted here
  Error(const Error &Other) = delete;
  ^
/home/asb/llvm-project/llvm/include/llvm/Support/Error.h:493:18: note: passing argument to parameter 'Err' here
  Expected(Error Err)
                 ^
1 warning and 3 errors generated.
[3/3711] Building CXX object tools/llvm-config/CMakeFiles/llvm-config.dir/llvm-config.cpp.o
ninja: build stopped: subcommand failed.

Changes:

  • Fix build warning and build error with clang

@asb Thanks for report that, there is no warning and build error when I build with GCC 7 (which is default compiler in Ubuntu 18.04), but I can reproduce that with clang 13, seems like I should switch my default compiler :P

eopXD added a subscriber: eopXD.Wed, Aug 25, 1:57 AM
eopXD added inline comments.
llvm/lib/Support/RISCVISAInfo.cpp
269

For naming consistency with parseArchString, maybe rename MArch to Arch?

Changes:

  • Remove unused argument MArch for getExtensionVersion.
kito-cheng marked an inline comment as done.Sun, Aug 29, 8:30 PM
kito-cheng added inline comments.
llvm/lib/Support/RISCVISAInfo.cpp
269

It turns out the argument isn't used anywhere...so I remove that.

Jim added inline comments.Mon, Aug 30, 8:49 PM
llvm/lib/Support/RISCVISAInfo.cpp
388

Does this need to check it is invalid if XLen is 64?

395

Why not get the version of i from SupportedExtensions?

kito-cheng marked an inline comment as done.

Changes:

  • Check feature combination is valid.
  • Change return type of parseFeatures to llvm::Expected<std::unique_ptr<RISCVISAInfo>>.
  • Set default extension version in getExtensionVersion.
kito-cheng marked 2 inline comments as done.Thu, Sep 2, 12:32 AM
kito-cheng added inline comments.
llvm/lib/Support/RISCVISAInfo.cpp
388

Add check, and parseFeatures might return Error now.

395

Updated :)

Jim added inline comments.Thu, Sep 2, 10:11 PM
llvm/lib/Support/RISCVISAInfo.cpp
417

Could this checking put before ISAInfo->addExtension...

kito-cheng marked 2 inline comments as done.

Changes:

  • Address Jim's comment.
kito-cheng marked an inline comment as done.Wed, Sep 8, 10:59 PM

I was trying to put this patch through its paces but it no longer applies. Can you please rebase it? It seems this is nearly there.

Seems like conflict with D108187, will update after testing :)

Changes:

  • Rebase to main.