[RISCV] Teach .option arch to support experimental extensions. #89727

yetingk · 2024-04-23T09:33:58Z

Previously .option arch denied extenions are not belongs to RISC-V features. But experimental features have experimental- prefix, so .option arch can not serve for experimental extension.
This patch uses the features of extensions to identify extension existance.

Previously .option arch denied extenions are not belongs to RISC-V features. But experimental features have experimental- prefix, so .option arch can not serve for experimental extension. This patch uses the features of extensions to identify extension existance.

llvmbot · 2024-04-23T09:34:31Z

@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-clang
@llvm/pr-subscribers-backend-risc-v

@llvm/pr-subscribers-mc

Author: Yeting Kuo (yetingk)

Changes

Previously .option arch denied extenions are not belongs to RISC-V features. But experimental features have experimental- prefix, so .option arch can not serve for experimental extension.
This patch uses the features of extensions to identify extension existance.

Full diff: https://github.com/llvm/llvm-project/pull/89727.diff

2 Files Affected:

(modified) llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp (+4-3)
(modified) llvm/test/MC/RISCV/option-arch.s (+8-1)

diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index 3f4a73ad89bf8a..80ff70f1095f4c 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -2824,8 +2824,9 @@ bool RISCVAsmParser::parseDirectiveOption() {
         break;
       }
 
-      auto Ext = llvm::lower_bound(RISCVFeatureKV, Arch);
-      if (Ext == std::end(RISCVFeatureKV) || StringRef(Ext->Key) != Arch ||
+      std::string &&Feature = RISCVISAInfo::getTargetFeatureForExtension(Arch);
+      auto Ext = llvm::lower_bound(RISCVFeatureKV, Feature);
+      if (Ext == std::end(RISCVFeatureKV) || StringRef(Ext->Key) != Feature ||
           !RISCVISAInfo::isSupportedExtension(Arch)) {
         if (isDigit(Arch.back()))
           return Error(
@@ -2834,7 +2835,7 @@ bool RISCVAsmParser::parseDirectiveOption() {
         return Error(Loc, "unknown extension feature");
       }
 
-      Args.emplace_back(Type, Ext->Key);
+      Args.emplace_back(Type, Arch.str());
 
       if (Type == RISCVOptionArchArgType::Plus) {
         FeatureBitset OldFeatureBits = STI->getFeatureBits();
diff --git a/llvm/test/MC/RISCV/option-arch.s b/llvm/test/MC/RISCV/option-arch.s
index 6ee133c7159a27..40675f9e4b434b 100644
--- a/llvm/test/MC/RISCV/option-arch.s
+++ b/llvm/test/MC/RISCV/option-arch.s
@@ -1,7 +1,7 @@
 # RUN: llvm-mc -triple riscv32 -show-encoding < %s \
 # RUN:   | FileCheck -check-prefixes=CHECK %s
 # RUN: llvm-mc -triple riscv32 -filetype=obj < %s \
-# RUN:   | llvm-objdump  --triple=riscv32 --mattr=+c,+m,+a,+f,+zba -d -M no-aliases - \
+# RUN:   | llvm-objdump  --triple=riscv32 --mattr=+c,+m,+a,+f,+zba,+experimental-zicfiss -d -M no-aliases - \
 # RUN:   | FileCheck -check-prefixes=CHECK-INST %s
 
 # Test '.option arch, +' and '.option arch, -' directive
@@ -78,6 +78,13 @@ lr.w t0, (t1)
 # CHECK: encoding: [0xb3,0x22,0x73,0x20]
 sh1add t0, t1, t2
 
+# Test experimental extension
+# CHECK: .option arch, +zicfiss
+.option arch, +zicfiss
+# CHECK-INST: sspopchk ra
+# CHECK: encoding: [0x73,0xc0,0xc0,0xcd]
+sspopchk ra
+
 # Test '.option arch, <arch-string>' directive
 # CHECK: .option arch, rv32i2p1_m2p0_a2p1_c2p0
 .option arch, rv32i2p1_m2p0_a2p1_c2p0

jrtc27 · 2024-04-23T09:44:50Z

I’m concerned about testing this; experimental extensions are, by policy, transient in LLVM, eventually either being promoted to non-experimental or being dropped, which means churn in this test every time the chosen extension is no longer present and experimental. What are your thoughts there?

jrtc27 · 2024-04-23T09:45:42Z

Also, does this obey -menable-experimental-extensions?

yetingk · 2024-04-23T13:05:05Z

Also, does this obey -menable-experimental-extensions?

IMO, I prefer this feature depends on -menable-experimental-extensions, since I think the option is to make user aware of experimental extensions.

jrtc27 · 2024-04-23T13:12:53Z

Also, does this obey -menable-experimental-extensions?

IMO, I prefer this feature depends on -menable-experimental-extensions.

Yes, I believe it should. My question is whether it currently does, especially given I don’t believe your current patch is testing it?

yetingk · 2024-04-23T13:15:55Z

It currently doesn't obey -menable-experimental-extensions. I will give a fix and tests for this.

topperc · 2024-04-24T05:00:37Z

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

@@ -51,6 +51,9 @@ STATISTIC(RISCVNumInstrsCompressed,

 static cl::opt<bool> AddBuildAttributes("riscv-add-build-attributes",
                                        cl::init(false));
+static cl::opt<bool>
+    DisableExperimentalExtension("riscv-disable-experimental-ext",


Why disable instead of enable?

Since only clang needs specific option to allows experimental extensions.

topperc · 2024-04-24T05:01:52Z

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

@@ -2824,8 +2827,12 @@ bool RISCVAsmParser::parseDirectiveOption() {
        break;
      }

-      auto Ext = llvm::lower_bound(RISCVFeatureKV, Arch);
-      if (Ext == std::end(RISCVFeatureKV) || StringRef(Ext->Key) != Arch ||
+      std::string &&Feature = RISCVISAInfo::getTargetFeatureForExtension(Arch);


Why an rvalue reference?

I think it might less call a string constructor() and just use the return string value. But acutally I am not really sure how does rvalue reference work.

I think you should just use std::string Feature. I think std::string &&Feature will just make a reference to a hidden temporary object.

topperc · 2024-04-25T15:28:49Z

Does this disable use of experimental extensions for a .option arch in an inline assembly block without -menable-experimental-extensions.

yetingk · 2024-04-25T16:56:44Z

Does this disable use of experimental extensions for a .option arch in an inline assembly block without -menable-experimental-extensions.

No. my patch could not block the case. I will try to think this problem after my sleep.

yetingk · 2024-04-26T05:04:14Z

I fix the issue by using feature to control the permission of experimental extension.

Does this disable use of experimental extensions for a .option arch in an inline assembly block without -menable-experimental-extensions.

topperc · 2024-04-29T22:18:34Z

clang/lib/Driver/ToolChains/Arch/RISCV.cpp

@@ -169,6 +169,9 @@ void riscv::getRISCVTargetFeatures(const Driver &D, const llvm::Triple &Triple,
    Features.push_back("-relax");
  }

+  if (!Args.hasArg(options::OPT_menable_experimental_extensions))
+    Features.push_back("+no-experimental-ext");


We also check OPT_menable_experimental_extensions in getArchFeatures and push a different feature when it is set. Do we need 2 extensions that are the opposite of each other?

Fixed. Only use +experimental feature.

topperc · 2024-05-01T05:56:39Z

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

@@ -51,7 +51,6 @@ STATISTIC(RISCVNumInstrsCompressed,

 static cl::opt<bool> AddBuildAttributes("riscv-add-build-attributes",
                                        cl::init(false));
-


Unrelated change?

topperc · 2024-05-01T05:59:09Z

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

+          StringRef(Feature).starts_with("experimental-"))
+        return Error(Loc, "Unexpected experimental extensions.");
+      auto Ext = llvm::lower_bound(RISCVFeatureKV, Feature);
+      if (Ext == std::end(RISCVFeatureKV) || StringRef(Ext->Key) != Feature ||
          !RISCVISAInfo::isSupportedExtension(Arch)) {


Do we still need the RISCVISAInfo::isSupportedExtension(Arch) check here? Did RISCVISAInfo::getTargetFeatureForExtension already take care of it?

Done. Thank you.

topperc

LGTM

yetingk · 2024-05-05T00:42:33Z

@topperc Sorry, there was a quick fix after the approve. getTargetFeatureForExtension could approve the extension with version. It may be better to allow them, but I am not sure the error message could be enough readable if we use wrong version number and is the format of output needed version number?

dyung · 2024-05-06T08:22:50Z

@yetingk, the two tests you added are failing on a couple of build bots:

Can you take a look and revert if you need time to investigate?

Followup to #89727

zmodem · 2024-05-06T10:39:49Z

6217abc should fix the test failures.

pogo59 · 2024-05-06T14:43:26Z

Driver tests are supposed to verify Driver behavior. That is, use -### and make sure the correct option is being passed down to the compiler. They should not be compiling anything.

yetingk · 2024-05-07T12:43:07Z

@zmodem very thank you for the help.

yetingk · 2024-05-07T12:54:19Z

@pogo59 Thank you for the advise. I will update the test cases.

PR llvm#89727 added the two test cases to verify `.option arch` should only work when having -menable-experimental-extensions. And the test idea could be splitted to 1. When having menable-experimental-extensions, clang passes +experimental. 2. `.option arch` only enabled when +experimental enabled. And we already had the two kind of test.

jrtc27 · 2024-05-07T15:36:02Z

clang/test/Driver/riscv-option-arch.c

@@ -0,0 +1,7 @@
+// RUN: %clang --target=riscv64 -menable-experimental-extensions -c -o /dev/null %s


Why are these clang tests? This is entirely testing llvm/ code. Never mind the weirdness of putting them in Driver/, nor the fact that they’re missing a REQUIRES: riscv-registered-target given they use the backend, they don’t belong in clang/ at all surely?

Yes. I am sorry that I didn't find out the obvious mistake. I am going to review my development process.

jrtc27 · 2024-05-07T15:36:10Z

clang/test/Driver/riscv-option-arch.c

@@ -0,0 +1,7 @@
+// RUN: %clang --target=riscv64 -menable-experimental-extensions -c -o /dev/null %s
+// RUN: ! %clang --target=riscv64 -c -o /dev/null %s 2>&1 | FileCheck -check-prefixes=CHECK-ERR %s


Use not rather than !

Thank you for the suggestion. I will fix this if #91324 could not remove the tests.

PR #89727 added the two test cases to verify `.option arch` should only work when having -menable-experimental-extensions. And the test idea could be splitted to 1. When having menable-experimental-extensions, clang passes +experimental. 2. `.option arch` only enabled when +experimental enabled. And we already had the two kind of tests.

MaskRay · 2024-05-13T20:05:03Z

The clang/test/Driver codegen tests might have been removed. Note: clang/test/Driver is to test how clangDriver passes options to cc1, not for sema/codegen/etc.

yetingk · 2024-05-14T02:35:19Z

The clang/test/Driver codegen tests might have been removed. Note: clang/test/Driver is to test how clangDriver passes options to cc1, not for sema/codegen/etc.

Thank you for the reminder.

yetingk requested review from asb, jrtc27 and topperc April 23, 2024 09:33

llvmbot added backend:RISC-V mc Machine (object) code labels Apr 23, 2024

yetingk requested a review from kito-cheng April 23, 2024 13:05

Make this pr obey menable-experimental-extensions.

471abce

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Apr 24, 2024

topperc reviewed Apr 24, 2024

View reviewed changes

Remove unneeded rvalue reference.

066445e

yetingk force-pushed the fix-option-arch branch from 1f6185b to 2414946 Compare April 29, 2024 07:41

topperc reviewed Apr 29, 2024

View reviewed changes

Use feature to allow/disallow experimental extensions.

a27e003

yetingk force-pushed the fix-option-arch branch from 2414946 to a27e003 Compare April 30, 2024 03:50

topperc reviewed May 1, 2024

View reviewed changes

Address Craig's comment.

8d098cb

topperc approved these changes May 3, 2024

View reviewed changes

fix: deny extensions with version number.

5b4035c

yetingk merged commit d70267f into llvm:main May 6, 2024

zmodem added a commit that referenced this pull request May 6, 2024

Add requires clause to risc-v clang driver tests

6217abc

Followup to #89727

yetingk mentioned this pull request May 7, 2024

[RISCV][NFC] Remove redundant test cases. #91324

Merged

jrtc27 reviewed May 7, 2024

View reviewed changes

		@@ -51,7 +51,6 @@ STATISTIC(RISCVNumInstrsCompressed,

		static cl::opt<bool> AddBuildAttributes("riscv-add-build-attributes",
		cl::init(false));

		@@ -0,0 +1,7 @@
		// RUN: %clang --target=riscv64 -menable-experimental-extensions -c -o /dev/null %s

		@@ -0,0 +1,7 @@
		// RUN: %clang --target=riscv64 -menable-experimental-extensions -c -o /dev/null %s
		// RUN: ! %clang --target=riscv64 -c -o /dev/null %s 2>&1 \| FileCheck -check-prefixes=CHECK-ERR %s

[RISCV] Teach .option arch to support experimental extensions. #89727

[RISCV] Teach .option arch to support experimental extensions. #89727

Uh oh!

Conversation

yetingk commented Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrtc27 commented Apr 23, 2024

Uh oh!

jrtc27 commented Apr 23, 2024

Uh oh!

yetingk commented Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrtc27 commented Apr 23, 2024

Uh oh!

yetingk commented Apr 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yetingk Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

topperc commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yetingk commented Apr 25, 2024

Uh oh!

yetingk commented Apr 26, 2024

Uh oh!

topperc Apr 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

topperc left a comment

Choose a reason for hiding this comment

Uh oh!

yetingk commented May 5, 2024

Uh oh!

dyung commented May 6, 2024

Uh oh!

zmodem commented May 6, 2024

Uh oh!

pogo59 commented May 6, 2024

Uh oh!

yetingk commented May 7, 2024

Uh oh!

yetingk commented May 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yetingk May 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

yetingk commented Apr 23, 2024 •

edited

Loading

llvmbot commented Apr 23, 2024 •

edited

Loading

yetingk commented Apr 23, 2024 •

edited

Loading

yetingk Apr 24, 2024 •

edited

Loading

topperc commented Apr 25, 2024 •

edited

Loading

topperc Apr 29, 2024 •

edited

Loading

yetingk May 7, 2024 •

edited

Loading