[mlir][amdgpu] Improve Chipset version utility #106169
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
kuhar
commented
Aug 27, 2024
kuhar
commented
- Fix an OOB access
- Add comparison operators
- Add documentation
- Add unit tests
* Fix an OOB access * Add comparison operators * Add documentation * Add unit tests
llvmbot
added
backend:AMDGPU
mlir:gpu
mlir
bazel
|
@llvm/pr-subscribers-mlir-amdgpu @llvm/pr-subscribers-mlir-gpu Author: Jakub Kuderski (kuhar) Changes
Full diff: https://github.com/llvm/llvm-project/pull/106169.diff 6 Files Affected:
diff --git a/mlir/include/mlir/Dialect/AMDGPU/Utils/Chipset.h b/mlir/include/mlir/Dialect/AMDGPU/Utils/Chipset.h
index 38e0ebe68f943b..0e2708b1efae03 100644
--- a/mlir/include/mlir/Dialect/AMDGPU/Utils/Chipset.h
+++ b/mlir/include/mlir/Dialect/AMDGPU/Utils/Chipset.h
@@ -9,19 +9,41 @@
#define MLIR_DIALECT_AMDGPU_UTILS_CHIPSET_H_
#include "mlir/Support/LLVM.h"
+#include <utility>
-namespace mlir {
-namespace amdgpu {
+namespace mlir::amdgpu {
+
+/// Represents the amdgpu gfx chipset version, e.g., gfx90a, gfx942, gfx1103.
+/// Note that the leading digits form a decimal number, while the last two
+/// digits for a hexadecimal number. For example:
+/// gfx942 --> major = 9, minor = 0x42
+/// gfx90a --> major = 9, minor = 0xa
+/// gfx1103 --> major = 10, minor = 0x3
struct Chipset {
Chipset() = default;
Chipset(unsigned majorVersion, unsigned minorVersion)
: majorVersion(majorVersion), minorVersion(minorVersion){};
+
+ /// Parses the chipset version string and returns the chipset on success, and
+ /// failure otherwise.
static FailureOr<Chipset> parse(StringRef name);
- unsigned majorVersion = 0;
- unsigned minorVersion = 0;
+ friend bool operator==(const Chipset &lhs, const Chipset &rhs) {
+ return lhs.majorVersion == rhs.majorVersion &&
+ lhs.minorVersion == rhs.minorVersion;
+ }
+ friend bool operator!=(const Chipset &lhs, const Chipset &rhs) {
+ return !(lhs == rhs);
+ }
+ friend bool operator<(const Chipset &lhs, const Chipset &rhs) {
+ return std::make_pair(lhs.majorVersion, lhs.minorVersion) <
+ std::make_pair(rhs.majorVersion, rhs.minorVersion);
+ }
+
+ unsigned majorVersion = 0; // The major version (decimal).
+ unsigned minorVersion = 0; // The minor version (hexadecimal).
};
-} // end namespace amdgpu
-} // end namespace mlir
+
+} // namespace mlir::amdgpu
#endif
diff --git a/mlir/lib/Dialect/AMDGPU/Utils/Chipset.cpp b/mlir/lib/Dialect/AMDGPU/Utils/Chipset.cpp
index 2540e1fb86d87d..fd15879d7b7ea0 100644
--- a/mlir/lib/Dialect/AMDGPU/Utils/Chipset.cpp
+++ b/mlir/lib/Dialect/AMDGPU/Utils/Chipset.cpp
@@ -1,4 +1,4 @@
-//===- Chipset.cpp - AMDGPU Chipset version struct parsing -----------===//
+//===- Chipset.cpp - AMDGPU Chipset version struct parsing ----------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
@@ -7,18 +7,20 @@
//===----------------------------------------------------------------------===//
#include "mlir/Dialect/AMDGPU/Utils/Chipset.h"
-#include "mlir/Support/LLVM.h"
#include "llvm/ADT/StringRef.h"
-using namespace mlir;
-using namespace mlir::amdgpu;
+namespace mlir::amdgpu {
FailureOr<Chipset> Chipset::parse(StringRef name) {
- if (!name.starts_with("gfx"))
+ if (!name.consume_front("gfx"))
return failure();
+ if (name.size() < 3)
+ return failure();
+
unsigned major = 0;
unsigned minor = 0;
- StringRef majorRef = name.drop_front(3).drop_back(2);
+
+ StringRef majorRef = name.drop_back(2);
StringRef minorRef = name.take_back(2);
if (majorRef.getAsInteger(10, major))
return failure();
@@ -26,3 +28,5 @@ FailureOr<Chipset> Chipset::parse(StringRef name) {
return failure();
return Chipset(major, minor);
}
+
+} // namespace mlir::amdgpu
diff --git a/mlir/unittests/Dialect/AMDGPU/AMDGPUUtilsTest.cpp b/mlir/unittests/Dialect/AMDGPU/AMDGPUUtilsTest.cpp
new file mode 100644
index 00000000000000..ba34e984d5643a
--- /dev/null
+++ b/mlir/unittests/Dialect/AMDGPU/AMDGPUUtilsTest.cpp
@@ -0,0 +1,62 @@
+//===- AMDGPUUtilsTest.cpp - Unit tests for AMDGPU dialect utils -----------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "mlir/Dialect/AMDGPU/Utils/Chipset.h"
+#include "gtest/gtest.h"
+
+namespace mlir::amdgpu {
+namespace {
+
+TEST(ChipsetTest, Parsing) {
+ FailureOr<Chipset> chipset = Chipset::parse("gfx90a");
+ ASSERT_TRUE(succeeded(chipset));
+ EXPECT_EQ(chipset->majorVersion, 9u);
+ EXPECT_EQ(chipset->minorVersion, 0x0au);
+
+ chipset = Chipset::parse("gfx940");
+ ASSERT_TRUE(succeeded(chipset));
+ EXPECT_EQ(chipset->majorVersion, 9u);
+ EXPECT_EQ(chipset->minorVersion, 0x40u);
+
+ chipset = Chipset::parse("gfx1103");
+ ASSERT_TRUE(succeeded(chipset));
+ EXPECT_EQ(chipset->majorVersion, 11u);
+ EXPECT_EQ(chipset->minorVersion, 0x03u);
+
+ ASSERT_TRUE(succeeded(chipset));
+ EXPECT_EQ(chipset->majorVersion, 11u);
+ EXPECT_EQ(chipset->minorVersion, 0x03u);
+}
+
+TEST(ChipsetTest, ParsingInvalid) {
+ EXPECT_TRUE(failed(Chipset::parse("navi33")));
+ EXPECT_TRUE(failed(Chipset::parse("rdna2")));
+ EXPECT_TRUE(failed(Chipset::parse("sm_80")));
+ EXPECT_TRUE(failed(Chipset::parse("GFX940")));
+ EXPECT_TRUE(failed(Chipset::parse("Gfx940")));
+ EXPECT_TRUE(failed(Chipset::parse("gfx9")));
+ EXPECT_TRUE(failed(Chipset::parse("gfx_940")));
+ EXPECT_TRUE(failed(Chipset::parse("gfx940_")));
+ EXPECT_TRUE(failed(Chipset::parse("gfxmeow")));
+ EXPECT_TRUE(failed(Chipset::parse("gfx1fff")));
+}
+
+TEST(ChipsetTest, Comparison) {
+ EXPECT_EQ(Chipset(9, 0x40), Chipset(9, 0x40));
+ EXPECT_NE(Chipset(9, 0x40), Chipset(9, 0x42));
+ EXPECT_NE(Chipset(9, 0x00), Chipset(10, 0x00));
+
+ EXPECT_LT(Chipset(9, 0x00), Chipset(10, 0x00));
+ EXPECT_LT(Chipset(9, 0x0a), Chipset(9, 0x42));
+ EXPECT_FALSE(Chipset(9, 0x42) < Chipset(9, 0x42));
+ EXPECT_FALSE(Chipset(9, 0x42) < Chipset(9, 0x40));
+}
+
+
+} // namespace
+} // namespace mlir::amdgpu
diff --git a/mlir/unittests/Dialect/AMDGPU/CMakeLists.txt b/mlir/unittests/Dialect/AMDGPU/CMakeLists.txt
new file mode 100644
index 00000000000000..d9a699e96288e8
--- /dev/null
+++ b/mlir/unittests/Dialect/AMDGPU/CMakeLists.txt
@@ -0,0 +1,7 @@
+add_mlir_unittest(MLIRAMDGPUTests
+ AMDGPUUtilsTest.cpp
+)
+target_link_libraries(MLIRAMDGPUTests
+ PRIVATE
+ MLIRAMDGPUUtils
+)
diff --git a/mlir/unittests/Dialect/CMakeLists.txt b/mlir/unittests/Dialect/CMakeLists.txt
index 90a75d5a46ad90..a5d4c48546e650 100644
--- a/mlir/unittests/Dialect/CMakeLists.txt
+++ b/mlir/unittests/Dialect/CMakeLists.txt
@@ -6,6 +6,7 @@ target_link_libraries(MLIRDialectTests
MLIRIR
MLIRDialect)
+add_subdirectory(AMDGPU)
add_subdirectory(ArmSME)
add_subdirectory(Index)
add_subdirectory(LLVMIR)
diff --git a/utils/bazel/llvm-project-overlay/mlir/unittests/BUILD.bazel b/utils/bazel/llvm-project-overlay/mlir/unittests/BUILD.bazel
index 7172beb4de9a62..a55c6f50118dcf 100644
--- a/utils/bazel/llvm-project-overlay/mlir/unittests/BUILD.bazel
+++ b/utils/bazel/llvm-project-overlay/mlir/unittests/BUILD.bazel
@@ -137,6 +137,19 @@ cc_test(
],
)
+cc_test(
+ name = "amdgpu_tests",
+ size = "small",
+ srcs = glob([
+ "Dialect/AMDGPU/*.cpp",
+ ]),
+ deps = [
+ "//mlir:AMDGPUUtils",
+ "//third-party/unittest:gtest",
+ "//third-party/unittest:gtest_main",
+ ],
+)
+
cc_test(
name = "memref_tests",
size = "small",
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
antiagainst
requested changes
Aug 28, 2024
| std::make_pair(rhs.majorVersion, rhs.minorVersion); | ||
| } | ||
|
|
||
| unsigned majorVersion = 0; // The major version (decimal). |
There was a problem hiding this comment.
I think there is also a stepping component: https://github.com/llvm/llvm-project/blob/f363e30/llvm/lib/TargetParser/TargetParser.cpp#L225. I sort of wonder the purpose of this function given we have that one--maybe we can wrap that up? It would cause a dependency on it though. But it's a separate issue I guess.
There was a problem hiding this comment.
This comment is non-blocking anyway.
There was a problem hiding this comment.
Good to know. It'd be useful to have a single interpretation in LLVM, but IMO changing the representation might be better for a separate PR.
antiagainst
approved these changes
Aug 28, 2024
5c4lar
pushed a commit
to 5c4lar/llvm-project
that referenced
this pull request
Aug 29, 2024
* Fix an OOB access * Add comparison operators * Add documentation * Add unit tests
kuhar
added a commit
that referenced
this pull request
Sep 9, 2024
Update the Chipset struct to follow the `IsaVersion` definition from llvm's `TargetParser`. This is a follow up to #106169 (comment). * Add the stepping version. Note: This may break downstream code that compares against the minor version directly. * Use comparisons with full Chipset version where possible. Note that we can't use the code in `TargetParser` directly because the chipset utility is outside of `mlir/Target` that re-exports llvm's target library.
VitaNuo
pushed a commit
to VitaNuo/llvm-project
that referenced
this pull request
Sep 12, 2024
Update the Chipset struct to follow the `IsaVersion` definition from llvm's `TargetParser`. This is a follow up to llvm#106169 (comment). * Add the stepping version. Note: This may break downstream code that compares against the minor version directly. * Use comparisons with full Chipset version where possible. Note that we can't use the code in `TargetParser` directly because the chipset utility is outside of `mlir/Target` that re-exports llvm's target library.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.