Skip to content

release/20.x: [RISCV] Allow Zicsr/Zifencei to duplicate with g (#136842) #137490

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 13, 2025

Conversation

llvmbot
Copy link
Member

@llvmbot llvmbot commented Apr 27, 2025

Backport 6c33735

Requested by: @wangpc-pp

@llvmbot
Copy link
Member Author

llvmbot commented Apr 27, 2025

@topperc What do you think about merging this PR to the release branch?

@llvmbot llvmbot requested a review from topperc April 27, 2025 03:20
@llvmbot llvmbot added clang Clang issues not falling into any other category backend:RISC-V labels Apr 27, 2025
@llvmbot
Copy link
Member Author

llvmbot commented Apr 27, 2025

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

@llvm/pr-subscribers-clang

Author: None (llvmbot)

Changes

Backport 6c33735

Requested by: @wangpc-pp


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

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+2)
  • (modified) llvm/lib/TargetParser/RISCVISAInfo.cpp (+15-3)
  • (modified) llvm/unittests/TargetParser/RISCVISAInfoTest.cpp (+8)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index b8f26ec9a5447..47ef2f80ac3f2 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1267,6 +1267,8 @@ RISC-V Support
 - The option ``-mcmodel=large`` for the large code model is supported.
 - Bump RVV intrinsic to version 1.0, the spec: https://github.com/riscv-non-isa/rvv-intrinsic-doc/releases/tag/v1.0.0-rc4
 
+- `Zicsr` / `Zifencei` are allowed to be duplicated in the presence of `g` in `-march`.
+
 CUDA/HIP Language Changes
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed a bug about overriding a constexpr pure-virtual member function with a non-constexpr virtual member function which causes compilation failure when including standard C++ header `format`.
diff --git a/llvm/lib/TargetParser/RISCVISAInfo.cpp b/llvm/lib/TargetParser/RISCVISAInfo.cpp
index c78d60fd86b3f..64ec411cb06e1 100644
--- a/llvm/lib/TargetParser/RISCVISAInfo.cpp
+++ b/llvm/lib/TargetParser/RISCVISAInfo.cpp
@@ -45,9 +45,8 @@ struct RISCVProfile {
 
 } // end anonymous namespace
 
-static const char *RISCVGImplications[] = {
-  "i", "m", "a", "f", "d", "zicsr", "zifencei"
-};
+static const char *RISCVGImplications[] = {"i", "m", "a", "f", "d"};
+static const char *RISCVGImplicationsZi[] = {"zicsr", "zifencei"};
 
 #define GET_SUPPORTED_EXTENSIONS
 #include "llvm/TargetParser/RISCVTargetParserDef.inc"
@@ -718,6 +717,19 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
     } while (!Ext.empty());
   }
 
+  // We add Zicsr/Zifenci as final to allow duplicated "zicsr"/"zifencei" like
+  // "rv64g_zicsr_zifencei".
+  if (Baseline == 'g') {
+    for (const char *Ext : RISCVGImplicationsZi) {
+      if (ISAInfo->Exts.count(Ext))
+        continue;
+
+      auto Version = findDefaultVersion(Ext);
+      assert(Version && "Default extension version not found?");
+      ISAInfo->Exts[std::string(Ext)] = {Version->Major, Version->Minor};
+    }
+  }
+
   return RISCVISAInfo::postProcessAndChecking(std::move(ISAInfo));
 }
 
diff --git a/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp b/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
index 7ebfcf915a7c5..5089bc0fd479a 100644
--- a/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
+++ b/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
@@ -507,6 +507,14 @@ TEST(ParseArchString, RejectsDoubleOrTrailingUnderscore) {
 }
 
 TEST(ParseArchString, RejectsDuplicateExtensionNames) {
+  // Zicsr/Zifencei are allowed to duplicate with "g".
+  ASSERT_THAT_EXPECTED(RISCVISAInfo::parseArchString("rv64g_zicsr", true),
+                       Succeeded());
+  ASSERT_THAT_EXPECTED(RISCVISAInfo::parseArchString("rv64g_zifencei", true),
+                       Succeeded());
+  ASSERT_THAT_EXPECTED(
+      RISCVISAInfo::parseArchString("rv64g_zicsr_zifencei", true), Succeeded());
+
   EXPECT_EQ(toString(RISCVISAInfo::parseArchString("rv64ii", true).takeError()),
             "invalid standard user-level extension 'i'");
   EXPECT_EQ(toString(RISCVISAInfo::parseArchString("rv32ee", true).takeError()),

@github-project-automation github-project-automation bot moved this from Needs Triage to Needs Merge in LLVM Release Status Apr 27, 2025
Copy link
Collaborator

@topperc topperc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tstellar tstellar moved this from Needs Merge to Needs Test Fix in LLVM Release Status Apr 29, 2025
@tstellar
Copy link
Collaborator

This has some failing tests.

@wangpc-pp
Copy link
Contributor

This has some failing tests.

The failure is not related to this PR I think, it is about compiler-rt/XRay:

/var/lib/buildkite-agent/builds/linux-56-59b8f5d88-6vh59-1/llvm-project/github-pull-requests/compiler-rt/test/xray/TestCases/Posix/basic-filtering.cpp:57:15: error: TRACE-NOT: excluded string found in input
// TRACE-NOT: - { type: 0, func-id: {{.*}}, function: {{.*filtered.*}}, {{.*}} }
              ^
<stdin>:10:2: note: found here
 - { type: 0, func-id: 1, function: 'filtered()', cpu: 0, thread: 2646461, process: 2646461, kind: function-enter, tsc: 1745726047361388709, data: '' }
 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Input file: <stdin>
Check file: /var/lib/buildkite-agent/builds/linux-56-59b8f5d88-6vh59-1/llvm-project/github-pull-requests/compiler-rt/test/xray/TestCases/Posix/basic-filtering.cpp

-dump-input=help explains the following input dump.

Input was:
<<<<<<
        .
        .
        .
        5:  constant-tsc: true 
        6:  nonstop-tsc: true 
        7:  cycle-frequency: 1000000000 
        8: records: 
        9:  - { type: 0, func-id: 4, function: main, cpu: 0, thread: 2646461, process: 2646461, kind: function-enter, tsc: 1745726047361383559, data: '' } 
       10:  - { type: 0, func-id: 1, function: 'filtered()', cpu: 0, thread: 2646461, process: 2646461, kind: function-enter, tsc: 1745726047361388709, data: '' } 
not:57      !~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~  error: no match expected
       11:  - { type: 0, func-id: 1, function: 'filtered()', cpu: 0, thread: 2646461, process: 2646461, kind: function-exit, tsc: 1745726047363841187, data: '' } 
       12:  - { type: 0, func-id: 3, function: 'always_shows()', cpu: 0, thread: 2646461, process: 2646461, kind: function-enter, tsc: 1745726047363842317, data: '' } 
       13:  - { type: 0, func-id: 3, function: 'always_shows()', cpu: 0, thread: 2646461, process: 2646461, kind: function-exit, tsc: 1745726047365914946, data: '' } 
       14:  - { type: 0, func-id: 4, function: main, cpu: 0, thread: 2646461, process: 2646461, kind: function-exit, tsc: 1745726047365916026, data: '' } 
       15: ... 
>>>>>>

@wangpc-pp
Copy link
Contributor

Thanks @tstellar! Now all checks are passed!

This matches GCC and we supported it in LLVM 17/18.

Fixes llvm#136803

(cherry picked from commit 6c33735)
@tstellar tstellar merged commit a708fb7 into llvm:release/20.x May 13, 2025
9 of 10 checks passed
@github-project-automation github-project-automation bot moved this from Needs Test Fix to Done in LLVM Release Status May 13, 2025
Copy link

@wangpc-pp (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

@wangpc-pp
Copy link
Contributor

@wangpc-pp (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

It is in the clang/docs/ReleaseNotes.rst: Zicsr / Zifencei are allowed to be duplicated in the presence of g in -march.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:RISC-V clang Clang issues not falling into any other category release:note
Projects
Development

Successfully merging this pull request may close these issues.

5 participants