Avoid Assertion Failure Using -fcs-profile-generate with distributed thin-lto #129736
+13
−2
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
These tests all deal with distributed thin-lto, and while their names indicate this, it is easier to find them in a common directory.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
You can test this locally with the following command:git diff -U0 --pickaxe-regex -S '([^a-zA-Z0-9#_-]undef[^a-zA-Z0-9_-]|UndefValue::get)' 6c87ec4f4d083a85ebcfbbda166ad4ba41d5da8d 4ef49b07453f7806ecd594bfaf570c4a6c5bc596 clang/test/CodeGen/distributed-thin-lto/cs-irpgo.c clang/lib/CodeGen/BackendUtil.cpp clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-backend-skip.ll clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-cfi-devirt.ll clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-cfi.ll clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-newpm.ll clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-objc-contract-pass.ll clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-supports-hot-cold-new.ll clang/test/CodeGen/distributed-thin-lto/thinlto-distributed.llThe following files introduce new uses of undef:
Undef is now deprecated and should only be used in the rare cases where no replacement is possible. For example, a load of uninitialized memory yields In tests, avoid using For example, this is considered a bad practice: define void @fn() {
...
br i1 undef, ...
}Please use the following instead: define void @fn(i1 %cond) {
...
br i1 %cond, ...
}Please refer to the Undefined Behavior Manual for more information. |
When using -fcs-generate-profile with distributed thin-lto without setting a profile output path, we hit an assertion constructing PGOOptions: https://github.com/llvm/llvm-project/blob/6041c745f32e8fd60ed24e29e7d919d8d1c87ca6/llvm/lib/Support/PGOOptions.cpp#L36 In non distributed thin-lto settings (local thin-lto for example), this profile output path is automatically set to a default value if -fcs-generate-profile is passed. This patch follows suit, using the default output path if one isn't set.
NuriAmari
force-pushed
the
nuriamari/dthin-lto-cs-irpgo
branch
from
cf1c88c to
4ef49b0
Compare
I haven't added new tests, I just moved existing ones. If the change looks otherwise reasonable, I'll try to figure out how to fix these tests. |
|
It's not entirely clear to me from the tests that exist if we are doing something wrong by passing flags in the manner shown here, IIUC Google already uses CSIRPGO with distributed thin-lto, but maybe more flags are used there. |
llvmbot
added
clang
NuriAmari
changed the title
|
@llvm/pr-subscribers-clang Author: Nuri Amari (NuriAmari) ChangesWhen using Using local thin-lto with LLD for MachO, we set the missing path automatically to a default value: https://reviews.llvm.org/D151589. In this fix we add the same behavior. Full diff: https://github.com/llvm/llvm-project/pull/129736.diff 9 Files Affected:
diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index 1750719e17670..3a41a412f2a8e 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -1334,7 +1334,10 @@ runThinLTOBackend(CompilerInstance &CI, ModuleSummaryIndex *CombinedIndex,
// Context sensitive profile.
if (CGOpts.hasProfileCSIRInstr()) {
Conf.RunCSIRInstr = true;
- Conf.CSIRProfile = std::move(CGOpts.InstrProfileOutput);
+ Conf.CSIRProfile = CGOpts.InstrProfileOutput.empty()
+ ? getDefaultProfileGenName()
+ : std::move(CGOpts.InstrProfileOutput);
+
} else if (CGOpts.hasProfileCSIRUse()) {
Conf.RunCSIRInstr = false;
Conf.CSIRProfile = std::move(CGOpts.ProfileInstrumentUsePath);
diff --git a/clang/test/CodeGen/distributed-thin-lto/cs-irpgo.c b/clang/test/CodeGen/distributed-thin-lto/cs-irpgo.c
new file mode 100644
index 0000000000000..28fc8d3519968
--- /dev/null
+++ b/clang/test/CodeGen/distributed-thin-lto/cs-irpgo.c
@@ -0,0 +1,8 @@
+// RUN: mkdir -p %t
+// RUN: %clang -target x86_64-apple-darwin -flto=thin %s -fcs-profile-generate -c -o %t/main.bc
+// RUN: llvm-lto2 run --thinlto-distributed-indexes %t/main.bc -r=%t/main.bc,_main,px -o %t/index
+// RUN: %clang -target x86_64-apple-darwin -fcs-profile-generate -fthinlto-index=%t/main.bc.thinlto.bc %t/main.bc -c -o main.o
+
+int main() {
+ return 0;
+}
diff --git a/clang/test/CodeGen/thinlto-distributed-backend-skip.ll b/clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-backend-skip.ll
similarity index 87%
rename from clang/test/CodeGen/thinlto-distributed-backend-skip.ll
rename to clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-backend-skip.ll
index d7b8225ee2693..62aa8aa8e7dc4 100644
--- a/clang/test/CodeGen/thinlto-distributed-backend-skip.ll
+++ b/clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-backend-skip.ll
@@ -6,7 +6,7 @@
; RUN: opt -thinlto-bc -o %t.o %s
; RUN: %clang_cc1 -triple x86_64-grtev4-linux-gnu \
-; RUN: -fthinlto-index=%S/Inputs/thinlto-distributed-backend-skip.bc \
+; RUN: -fthinlto-index=%S/../Inputs/thinlto-distributed-backend-skip.bc \
; RUN: -emit-llvm -o - -x ir %t.o | FileCheck %s
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
diff --git a/clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll b/clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-cfi-devirt.ll
similarity index 100%
rename from clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll
rename to clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-cfi-devirt.ll
diff --git a/clang/test/CodeGen/thinlto-distributed-cfi.ll b/clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-cfi.ll
similarity index 100%
rename from clang/test/CodeGen/thinlto-distributed-cfi.ll
rename to clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-cfi.ll
diff --git a/clang/test/CodeGen/thinlto-distributed-newpm.ll b/clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-newpm.ll
similarity index 100%
rename from clang/test/CodeGen/thinlto-distributed-newpm.ll
rename to clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-newpm.ll
diff --git a/clang/test/CodeGen/thinlto-distributed-objc-contract-pass.ll b/clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-objc-contract-pass.ll
similarity index 100%
rename from clang/test/CodeGen/thinlto-distributed-objc-contract-pass.ll
rename to clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-objc-contract-pass.ll
diff --git a/clang/test/CodeGen/thinlto-distributed-supports-hot-cold-new.ll b/clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-supports-hot-cold-new.ll
similarity index 100%
rename from clang/test/CodeGen/thinlto-distributed-supports-hot-cold-new.ll
rename to clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-supports-hot-cold-new.ll
diff --git a/clang/test/CodeGen/thinlto-distributed.ll b/clang/test/CodeGen/distributed-thin-lto/thinlto-distributed.ll
similarity index 100%
rename from clang/test/CodeGen/thinlto-distributed.ll
rename to clang/test/CodeGen/distributed-thin-lto/thinlto-distributed.ll
|
|
@llvm/pr-subscribers-clang-codegen Author: Nuri Amari (NuriAmari) ChangesWhen using Using local thin-lto with LLD for MachO, we set the missing path automatically to a default value: https://reviews.llvm.org/D151589. In this fix we add the same behavior. Full diff: https://github.com/llvm/llvm-project/pull/129736.diff 9 Files Affected:
diff --git a/clang/lib/CodeGen/BackendUtil.cpp b/clang/lib/CodeGen/BackendUtil.cpp
index 1750719e17670..3a41a412f2a8e 100644
--- a/clang/lib/CodeGen/BackendUtil.cpp
+++ b/clang/lib/CodeGen/BackendUtil.cpp
@@ -1334,7 +1334,10 @@ runThinLTOBackend(CompilerInstance &CI, ModuleSummaryIndex *CombinedIndex,
// Context sensitive profile.
if (CGOpts.hasProfileCSIRInstr()) {
Conf.RunCSIRInstr = true;
- Conf.CSIRProfile = std::move(CGOpts.InstrProfileOutput);
+ Conf.CSIRProfile = CGOpts.InstrProfileOutput.empty()
+ ? getDefaultProfileGenName()
+ : std::move(CGOpts.InstrProfileOutput);
+
} else if (CGOpts.hasProfileCSIRUse()) {
Conf.RunCSIRInstr = false;
Conf.CSIRProfile = std::move(CGOpts.ProfileInstrumentUsePath);
diff --git a/clang/test/CodeGen/distributed-thin-lto/cs-irpgo.c b/clang/test/CodeGen/distributed-thin-lto/cs-irpgo.c
new file mode 100644
index 0000000000000..28fc8d3519968
--- /dev/null
+++ b/clang/test/CodeGen/distributed-thin-lto/cs-irpgo.c
@@ -0,0 +1,8 @@
+// RUN: mkdir -p %t
+// RUN: %clang -target x86_64-apple-darwin -flto=thin %s -fcs-profile-generate -c -o %t/main.bc
+// RUN: llvm-lto2 run --thinlto-distributed-indexes %t/main.bc -r=%t/main.bc,_main,px -o %t/index
+// RUN: %clang -target x86_64-apple-darwin -fcs-profile-generate -fthinlto-index=%t/main.bc.thinlto.bc %t/main.bc -c -o main.o
+
+int main() {
+ return 0;
+}
diff --git a/clang/test/CodeGen/thinlto-distributed-backend-skip.ll b/clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-backend-skip.ll
similarity index 87%
rename from clang/test/CodeGen/thinlto-distributed-backend-skip.ll
rename to clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-backend-skip.ll
index d7b8225ee2693..62aa8aa8e7dc4 100644
--- a/clang/test/CodeGen/thinlto-distributed-backend-skip.ll
+++ b/clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-backend-skip.ll
@@ -6,7 +6,7 @@
; RUN: opt -thinlto-bc -o %t.o %s
; RUN: %clang_cc1 -triple x86_64-grtev4-linux-gnu \
-; RUN: -fthinlto-index=%S/Inputs/thinlto-distributed-backend-skip.bc \
+; RUN: -fthinlto-index=%S/../Inputs/thinlto-distributed-backend-skip.bc \
; RUN: -emit-llvm -o - -x ir %t.o | FileCheck %s
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
diff --git a/clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll b/clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-cfi-devirt.ll
similarity index 100%
rename from clang/test/CodeGen/thinlto-distributed-cfi-devirt.ll
rename to clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-cfi-devirt.ll
diff --git a/clang/test/CodeGen/thinlto-distributed-cfi.ll b/clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-cfi.ll
similarity index 100%
rename from clang/test/CodeGen/thinlto-distributed-cfi.ll
rename to clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-cfi.ll
diff --git a/clang/test/CodeGen/thinlto-distributed-newpm.ll b/clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-newpm.ll
similarity index 100%
rename from clang/test/CodeGen/thinlto-distributed-newpm.ll
rename to clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-newpm.ll
diff --git a/clang/test/CodeGen/thinlto-distributed-objc-contract-pass.ll b/clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-objc-contract-pass.ll
similarity index 100%
rename from clang/test/CodeGen/thinlto-distributed-objc-contract-pass.ll
rename to clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-objc-contract-pass.ll
diff --git a/clang/test/CodeGen/thinlto-distributed-supports-hot-cold-new.ll b/clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-supports-hot-cold-new.ll
similarity index 100%
rename from clang/test/CodeGen/thinlto-distributed-supports-hot-cold-new.ll
rename to clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-supports-hot-cold-new.ll
diff --git a/clang/test/CodeGen/thinlto-distributed.ll b/clang/test/CodeGen/distributed-thin-lto/thinlto-distributed.ll
similarity index 100%
rename from clang/test/CodeGen/thinlto-distributed.ll
rename to clang/test/CodeGen/distributed-thin-lto/thinlto-distributed.ll
|
ellishg
reviewed
Mar 4, 2025
There was a problem hiding this comment.
Should we remove the thinlto-distributed from the filenames since the directory is named distributed-thin-lto?
ellishg
reviewed
Mar 5, 2025
Comment on lines
+1337
to
+1340
| Conf.CSIRProfile = CGOpts.InstrProfileOutput.empty() | ||
| ? getDefaultProfileGenName() | ||
| : std::move(CGOpts.InstrProfileOutput); | ||
|
|
There was a problem hiding this comment.
Looks like we use getProfileGenName() now.
Suggested change
| Conf.CSIRProfile = CGOpts.InstrProfileOutput.empty() | |
| ? getDefaultProfileGenName() | |
| : std::move(CGOpts.InstrProfileOutput); | |
| Conf.CSIRProfile = getProfileGenName(CGOpts) |
teresajohnson
approved these changes
Mar 6, 2025
There was a problem hiding this comment.
lgtm with the change suggested by @ellishg to use getProfileGenName. I believe we always specify a profile filename using the = form of the option, which is why we have not encountered this issue.
| @@ -6,7 +6,7 @@ | |||
| ; RUN: opt -thinlto-bc -o %t.o %s | |||
|
|
|||
| ; RUN: %clang_cc1 -triple x86_64-grtev4-linux-gnu \ | |||
| ; RUN: -fthinlto-index=%S/Inputs/thinlto-distributed-backend-skip.bc \ | |||
| ; RUN: -fthinlto-index=%S/../Inputs/thinlto-distributed-backend-skip.bc \ | |||
There was a problem hiding this comment.
Why is this change needed?
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.
When using
-fcs-generate-profilewith distributed thin-lto in the same fashion we do for local thin-lto, we hit the following assertion:llvm-project/llvm/lib/Support/PGOOptions.cpp
Line 36 in 6041c74
Using local thin-lto with LLD for MachO, we set the missing path automatically to a default value: https://reviews.llvm.org/D151589. In this fix we add the same behavior.