[llvm-exegesis] Use hex values when parsing memory mappings #72620

boomanaiden154 · 2023-11-17T08:01:46Z

Currently, the address in the LLVM-EXEGESIS-MEM-MAP annotation is parsed in decimal format. This makes it inconsistent with the rest of the program and inconsistent with the way most memory addresses are presented as they are presented in hexadeicmal. This patch changes this behavior so that llvm-exegesis automatically parses the address in LLVM-EXEGESIS-MEM-MAP annotations as a hexadeicmal value.

llvmbot · 2023-11-17T08:02:15Z

@llvm/pr-subscribers-tools-llvm-exegesis

Author: Aiden Grossman (boomanaiden154)

Changes

Currently, the address in the LLVM-EXEGESIS-MEM-MAP annotation is parsed in decimal format. This makes it inconsistent with the rest of the program and inconsistent with the way most memory addresses are presented as they are presented in hexadeicmal. This patch changes this behavior so that llvm-exegesis automatically parses the address in LLVM-EXEGESIS-MEM-MAP annotations as a hexadeicmal value.

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

5 Files Affected:

(modified) llvm/docs/CommandGuide/llvm-exegesis.rst (+1-1)
(modified) llvm/test/tools/llvm-exegesis/X86/latency/memory-annotations-livein.s (+2-2)
(modified) llvm/test/tools/llvm-exegesis/X86/latency/memory-annotations.s (+2-2)
(modified) llvm/tools/llvm-exegesis/lib/SnippetFile.cpp (+1-1)
(modified) llvm/unittests/tools/llvm-exegesis/X86/SnippetFileTest.cpp (+11-11)

diff --git a/llvm/docs/CommandGuide/llvm-exegesis.rst b/llvm/docs/CommandGuide/llvm-exegesis.rst
index 874bae82a1029af..b886a3df3ebbcaf 100644
--- a/llvm/docs/CommandGuide/llvm-exegesis.rst
+++ b/llvm/docs/CommandGuide/llvm-exegesis.rst
@@ -77,7 +77,7 @@ properly.
 * `LLVM-EXEGESIS-MEM-MAP <value name> <address>` - This annotation allows for
   mapping previously defined memory definitions into the execution context of a
   process. The value name refers to a previously defined memory definition and
-  the address is a decimal number that specifies the address the memory
+  the address is a hex value that specifies the address the memory
   definition should start at. Note that a single memory definition can be
   mapped multiple times. Using this annotation requires the subprocess
   execution mode.
diff --git a/llvm/test/tools/llvm-exegesis/X86/latency/memory-annotations-livein.s b/llvm/test/tools/llvm-exegesis/X86/latency/memory-annotations-livein.s
index 5d9171959b6afed..31d1f603f52bac8 100644
--- a/llvm/test/tools/llvm-exegesis/X86/latency/memory-annotations-livein.s
+++ b/llvm/test/tools/llvm-exegesis/X86/latency/memory-annotations-livein.s
@@ -12,8 +12,8 @@
 # ALLOW_RETRIES: 2
 
 # LLVM-EXEGESIS-MEM-DEF test1 4096 2147483647
-# LLVM-EXEGESIS-MEM-MAP test1 1048576
+# LLVM-EXEGESIS-MEM-MAP test1 100000
 # LLVM-EXEGESIS-LIVEIN R14
 
-movq $1048576, %rax
+movq $0x100000, %rax
 movq %r14, (%rax)
diff --git a/llvm/test/tools/llvm-exegesis/X86/latency/memory-annotations.s b/llvm/test/tools/llvm-exegesis/X86/latency/memory-annotations.s
index 735deb40243c65a..18f086282871d5f 100644
--- a/llvm/test/tools/llvm-exegesis/X86/latency/memory-annotations.s
+++ b/llvm/test/tools/llvm-exegesis/X86/latency/memory-annotations.s
@@ -14,7 +14,7 @@
 # ALLOW_RETRIES: 2
 
 # LLVM-EXEGESIS-MEM-DEF test1 4096 2147483647
-# LLVM-EXEGESIS-MEM-MAP test1 1048576
+# LLVM-EXEGESIS-MEM-MAP test1 100000
 
-movq $1048576, %rax
+movq $0x100000, %rax
 movq (%rax), %rdi
diff --git a/llvm/tools/llvm-exegesis/lib/SnippetFile.cpp b/llvm/tools/llvm-exegesis/lib/SnippetFile.cpp
index d85a9f190655a5b..2deb2b78ca58e26 100644
--- a/llvm/tools/llvm-exegesis/lib/SnippetFile.cpp
+++ b/llvm/tools/llvm-exegesis/lib/SnippetFile.cpp
@@ -117,7 +117,7 @@ class BenchmarkCodeStreamer : public MCStreamer, public AsmCommentConsumer {
       }
       MemoryMapping MemMap;
       MemMap.MemoryValueName = Parts[0].trim().str();
-      MemMap.Address = std::stol(Parts[1].trim().str());
+      MemMap.Address = std::stol(Parts[1].trim().str(), nullptr, 16);
       // validate that the annotation refers to an already existing memory
       // definition
       auto MemValIT = Result->Key.MemoryValues.find(Parts[0].trim().str());
diff --git a/llvm/unittests/tools/llvm-exegesis/X86/SnippetFileTest.cpp b/llvm/unittests/tools/llvm-exegesis/X86/SnippetFileTest.cpp
index b05a713351514ba..c7435ade6cb5b05 100644
--- a/llvm/unittests/tools/llvm-exegesis/X86/SnippetFileTest.cpp
+++ b/llvm/unittests/tools/llvm-exegesis/X86/SnippetFileTest.cpp
@@ -147,9 +147,9 @@ TEST_F(X86SnippetFileTest, NoAsmStreamer) {
 TEST_F(X86SnippetFileTest, MemoryDefinitionTestSingleDef) {
   auto Snippets = TestCommon(R"(
     # LLVM-EXEGESIS-MEM-DEF test1 4096 ff
-    # LLVM-EXEGESIS-MEM-MAP test1 8192
-    # LLVM-EXEGESIS-MEM-MAP test1 16384
-    movq $8192, %r10
+    # LLVM-EXEGESIS-MEM-MAP test1 2000
+    # LLVM-EXEGESIS-MEM-MAP test1 4000
+    movq $0x2000, %r10
     movq (%r10), %r11
   )");
   EXPECT_FALSE((bool)Snippets.takeError());
@@ -158,16 +158,16 @@ TEST_F(X86SnippetFileTest, MemoryDefinitionTestSingleDef) {
   ASSERT_THAT(Snippet.Key.MemoryValues,
               UnorderedElementsAre(MemoryDefinitionIs("test1", 255, 4096)));
   ASSERT_THAT(Snippet.Key.MemoryMappings,
-              ElementsAre(MemoryMappingIs(8192, "test1"),
-                          MemoryMappingIs(16384, "test1")));
+              ElementsAre(MemoryMappingIs(0x2000, "test1"),
+                          MemoryMappingIs(0x4000, "test1")));
 }
 
 TEST_F(X86SnippetFileTest, MemoryDefinitionsTestTwoDef) {
   auto Snippets = TestCommon(R"(
     # LLVM-EXEGESIS-MEM-DEF test1 4096 ff
     # LLVM-EXEGESIS-MEM-DEF test2 4096 100
-    # LLVM-EXEGESIS-MEM-MAP test1 8192
-    # LLVM-EXEGESIS-MEM-MAP test2 16384
+    # LLVM-EXEGESIS-MEM-MAP test1 2000
+    # LLVM-EXEGESIS-MEM-MAP test2 4000
     movq $8192, %r10
     movq (%r10), %r11
   )");
@@ -178,8 +178,8 @@ TEST_F(X86SnippetFileTest, MemoryDefinitionsTestTwoDef) {
               UnorderedElementsAre(MemoryDefinitionIs("test1", 255, 4096),
                                    MemoryDefinitionIs("test2", 256, 4096)));
   ASSERT_THAT(Snippet.Key.MemoryMappings,
-              ElementsAre(MemoryMappingIs(8192, "test1"),
-                          MemoryMappingIs(16384, "test2")));
+              ElementsAre(MemoryMappingIs(0x2000, "test1"),
+                          MemoryMappingIs(0x4000, "test2")));
 }
 
 TEST_F(X86SnippetFileTest, MemoryDefinitionMissingParameter) {
@@ -202,7 +202,7 @@ TEST_F(X86SnippetFileTest, MemoryMappingMissingParameters) {
 
 TEST_F(X86SnippetFileTest, MemoryMappingNoDefinition) {
   auto Error = TestCommon(R"(
-    # LLVM-EXEGESIS-MEM-MAP test1 4096
+    # LLVM-EXEGESIS-MEM-MAP test1 2000
   )")
                    .takeError();
   EXPECT_TRUE((bool)Error);
@@ -211,7 +211,7 @@ TEST_F(X86SnippetFileTest, MemoryMappingNoDefinition) {
 
 TEST_F(X86SnippetFileTest, IncompatibleExecutorMode) {
   auto Error = TestCommon(R"(
-    # LLVM-EXEGESIS-MEM-MAP test1 4096
+    # LLVM-EXEGESIS-MEM-MAP test1 2000
   )")
                    .takeError();
   EXPECT_TRUE((bool)Error);

llvmbot · 2023-11-17T08:02:16Z

@llvm/pr-subscribers-llvm-binary-utilities

Author: Aiden Grossman (boomanaiden154)

Changes

Currently, the address in the LLVM-EXEGESIS-MEM-MAP annotation is parsed in decimal format. This makes it inconsistent with the rest of the program and inconsistent with the way most memory addresses are presented as they are presented in hexadeicmal. This patch changes this behavior so that llvm-exegesis automatically parses the address in LLVM-EXEGESIS-MEM-MAP annotations as a hexadeicmal value.

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

5 Files Affected:

(modified) llvm/docs/CommandGuide/llvm-exegesis.rst (+1-1)
(modified) llvm/test/tools/llvm-exegesis/X86/latency/memory-annotations-livein.s (+2-2)
(modified) llvm/test/tools/llvm-exegesis/X86/latency/memory-annotations.s (+2-2)
(modified) llvm/tools/llvm-exegesis/lib/SnippetFile.cpp (+1-1)
(modified) llvm/unittests/tools/llvm-exegesis/X86/SnippetFileTest.cpp (+11-11)

diff --git a/llvm/docs/CommandGuide/llvm-exegesis.rst b/llvm/docs/CommandGuide/llvm-exegesis.rst
index 874bae82a1029af..b886a3df3ebbcaf 100644
--- a/llvm/docs/CommandGuide/llvm-exegesis.rst
+++ b/llvm/docs/CommandGuide/llvm-exegesis.rst
@@ -77,7 +77,7 @@ properly.
 * `LLVM-EXEGESIS-MEM-MAP <value name> <address>` - This annotation allows for
   mapping previously defined memory definitions into the execution context of a
   process. The value name refers to a previously defined memory definition and
-  the address is a decimal number that specifies the address the memory
+  the address is a hex value that specifies the address the memory
   definition should start at. Note that a single memory definition can be
   mapped multiple times. Using this annotation requires the subprocess
   execution mode.
diff --git a/llvm/test/tools/llvm-exegesis/X86/latency/memory-annotations-livein.s b/llvm/test/tools/llvm-exegesis/X86/latency/memory-annotations-livein.s
index 5d9171959b6afed..31d1f603f52bac8 100644
--- a/llvm/test/tools/llvm-exegesis/X86/latency/memory-annotations-livein.s
+++ b/llvm/test/tools/llvm-exegesis/X86/latency/memory-annotations-livein.s
@@ -12,8 +12,8 @@
 # ALLOW_RETRIES: 2
 
 # LLVM-EXEGESIS-MEM-DEF test1 4096 2147483647
-# LLVM-EXEGESIS-MEM-MAP test1 1048576
+# LLVM-EXEGESIS-MEM-MAP test1 100000
 # LLVM-EXEGESIS-LIVEIN R14
 
-movq $1048576, %rax
+movq $0x100000, %rax
 movq %r14, (%rax)
diff --git a/llvm/test/tools/llvm-exegesis/X86/latency/memory-annotations.s b/llvm/test/tools/llvm-exegesis/X86/latency/memory-annotations.s
index 735deb40243c65a..18f086282871d5f 100644
--- a/llvm/test/tools/llvm-exegesis/X86/latency/memory-annotations.s
+++ b/llvm/test/tools/llvm-exegesis/X86/latency/memory-annotations.s
@@ -14,7 +14,7 @@
 # ALLOW_RETRIES: 2
 
 # LLVM-EXEGESIS-MEM-DEF test1 4096 2147483647
-# LLVM-EXEGESIS-MEM-MAP test1 1048576
+# LLVM-EXEGESIS-MEM-MAP test1 100000
 
-movq $1048576, %rax
+movq $0x100000, %rax
 movq (%rax), %rdi
diff --git a/llvm/tools/llvm-exegesis/lib/SnippetFile.cpp b/llvm/tools/llvm-exegesis/lib/SnippetFile.cpp
index d85a9f190655a5b..2deb2b78ca58e26 100644
--- a/llvm/tools/llvm-exegesis/lib/SnippetFile.cpp
+++ b/llvm/tools/llvm-exegesis/lib/SnippetFile.cpp
@@ -117,7 +117,7 @@ class BenchmarkCodeStreamer : public MCStreamer, public AsmCommentConsumer {
       }
       MemoryMapping MemMap;
       MemMap.MemoryValueName = Parts[0].trim().str();
-      MemMap.Address = std::stol(Parts[1].trim().str());
+      MemMap.Address = std::stol(Parts[1].trim().str(), nullptr, 16);
       // validate that the annotation refers to an already existing memory
       // definition
       auto MemValIT = Result->Key.MemoryValues.find(Parts[0].trim().str());
diff --git a/llvm/unittests/tools/llvm-exegesis/X86/SnippetFileTest.cpp b/llvm/unittests/tools/llvm-exegesis/X86/SnippetFileTest.cpp
index b05a713351514ba..c7435ade6cb5b05 100644
--- a/llvm/unittests/tools/llvm-exegesis/X86/SnippetFileTest.cpp
+++ b/llvm/unittests/tools/llvm-exegesis/X86/SnippetFileTest.cpp
@@ -147,9 +147,9 @@ TEST_F(X86SnippetFileTest, NoAsmStreamer) {
 TEST_F(X86SnippetFileTest, MemoryDefinitionTestSingleDef) {
   auto Snippets = TestCommon(R"(
     # LLVM-EXEGESIS-MEM-DEF test1 4096 ff
-    # LLVM-EXEGESIS-MEM-MAP test1 8192
-    # LLVM-EXEGESIS-MEM-MAP test1 16384
-    movq $8192, %r10
+    # LLVM-EXEGESIS-MEM-MAP test1 2000
+    # LLVM-EXEGESIS-MEM-MAP test1 4000
+    movq $0x2000, %r10
     movq (%r10), %r11
   )");
   EXPECT_FALSE((bool)Snippets.takeError());
@@ -158,16 +158,16 @@ TEST_F(X86SnippetFileTest, MemoryDefinitionTestSingleDef) {
   ASSERT_THAT(Snippet.Key.MemoryValues,
               UnorderedElementsAre(MemoryDefinitionIs("test1", 255, 4096)));
   ASSERT_THAT(Snippet.Key.MemoryMappings,
-              ElementsAre(MemoryMappingIs(8192, "test1"),
-                          MemoryMappingIs(16384, "test1")));
+              ElementsAre(MemoryMappingIs(0x2000, "test1"),
+                          MemoryMappingIs(0x4000, "test1")));
 }
 
 TEST_F(X86SnippetFileTest, MemoryDefinitionsTestTwoDef) {
   auto Snippets = TestCommon(R"(
     # LLVM-EXEGESIS-MEM-DEF test1 4096 ff
     # LLVM-EXEGESIS-MEM-DEF test2 4096 100
-    # LLVM-EXEGESIS-MEM-MAP test1 8192
-    # LLVM-EXEGESIS-MEM-MAP test2 16384
+    # LLVM-EXEGESIS-MEM-MAP test1 2000
+    # LLVM-EXEGESIS-MEM-MAP test2 4000
     movq $8192, %r10
     movq (%r10), %r11
   )");
@@ -178,8 +178,8 @@ TEST_F(X86SnippetFileTest, MemoryDefinitionsTestTwoDef) {
               UnorderedElementsAre(MemoryDefinitionIs("test1", 255, 4096),
                                    MemoryDefinitionIs("test2", 256, 4096)));
   ASSERT_THAT(Snippet.Key.MemoryMappings,
-              ElementsAre(MemoryMappingIs(8192, "test1"),
-                          MemoryMappingIs(16384, "test2")));
+              ElementsAre(MemoryMappingIs(0x2000, "test1"),
+                          MemoryMappingIs(0x4000, "test2")));
 }
 
 TEST_F(X86SnippetFileTest, MemoryDefinitionMissingParameter) {
@@ -202,7 +202,7 @@ TEST_F(X86SnippetFileTest, MemoryMappingMissingParameters) {
 
 TEST_F(X86SnippetFileTest, MemoryMappingNoDefinition) {
   auto Error = TestCommon(R"(
-    # LLVM-EXEGESIS-MEM-MAP test1 4096
+    # LLVM-EXEGESIS-MEM-MAP test1 2000
   )")
                    .takeError();
   EXPECT_TRUE((bool)Error);
@@ -211,7 +211,7 @@ TEST_F(X86SnippetFileTest, MemoryMappingNoDefinition) {
 
 TEST_F(X86SnippetFileTest, IncompatibleExecutorMode) {
   auto Error = TestCommon(R"(
-    # LLVM-EXEGESIS-MEM-MAP test1 4096
+    # LLVM-EXEGESIS-MEM-MAP test1 2000
   )")
                    .takeError();
   EXPECT_TRUE((bool)Error);

boomanaiden154 · 2023-11-17T08:03:20Z

@ondrasej

There are relatively few (known) users of the memory annotations in llvm-exegesis, so I don't believe this change should have any significant impact on existing users and will probably make it easier to use for those existing users (have had at least one question about that so far).

boomanaiden154 · 2023-11-17T08:07:48Z

Additionally, we might want to do the same for the memory definition sizes. That would probably be for the best given everything else will be in hex after this point for consistency purposes.

legrosbuffle · 2023-11-17T12:24:02Z

As this change is not backwards compatible we might be breaking people with no way for them/us to know. I see two options:

Support both decimal and hexadecimal. Prefix the latter with 0x
Add a new deirective LLVM-EXEGESIS-MEM-MAP-HEX that reads hex values.

boomanaiden154 · 2023-11-17T20:27:05Z

Reference

Right, this change won't be backwards compatible. It was a fairly significant oversight on my part when doing the implementation, and I'd really like to get this fixed properly (along with the definition sizes) early on while there are relatively few users of this feature and relatively little external tooling that relies on it.

I think just doing the change now and adding a note in the release notes at this point would keep everything clean without having to introduce additional annotations/keep the very unintuitive interface that we have currently.

legrosbuffle · 2023-11-20T07:30:42Z

As far as I'd like introducing name churn I don't think we can rely on people reading the release notes for this. We can't know whether someone is relying on these values; doing things properly is probably less work than people debugging the issue on their side.

The 0x opetion avoids changing the name and actually makes it clear that the value is hex.

boomanaiden154 · 2023-11-21T09:03:18Z

That's a good point. I'll close this patch for now and probably open up a follow-up patch allowing for the parsing of mapping addresses with 0x, and maybe the other values too for consistency.

Thanks for your input here.

boomanaiden154 requested review from gchatelet and legrosbuffle November 17, 2023 08:01

llvmbot added tools:llvm-exegesis llvm:binary-utilities labels Nov 17, 2023

boomanaiden154 mentioned this pull request Nov 17, 2023

Add script to generate input code snippets for llvm-exegesis google/gematria#28

Merged

boomanaiden154 closed this Nov 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[llvm-exegesis] Use hex values when parsing memory mappings #72620

[llvm-exegesis] Use hex values when parsing memory mappings #72620

boomanaiden154 commented Nov 17, 2023

llvmbot commented Nov 17, 2023

llvmbot commented Nov 17, 2023

boomanaiden154 commented Nov 17, 2023

boomanaiden154 commented Nov 17, 2023

legrosbuffle commented Nov 17, 2023

boomanaiden154 commented Nov 17, 2023

legrosbuffle commented Nov 20, 2023

boomanaiden154 commented Nov 21, 2023

[llvm-exegesis] Use hex values when parsing memory mappings #72620

[llvm-exegesis] Use hex values when parsing memory mappings #72620

Conversation

boomanaiden154 commented Nov 17, 2023

llvmbot commented Nov 17, 2023

llvmbot commented Nov 17, 2023

boomanaiden154 commented Nov 17, 2023

boomanaiden154 commented Nov 17, 2023

legrosbuffle commented Nov 17, 2023

boomanaiden154 commented Nov 17, 2023

legrosbuffle commented Nov 20, 2023

boomanaiden154 commented Nov 21, 2023