Avoid Assertion Failure Using -fcs-profile-generate with distributed thin-lto #129736
Conversation
This file contains hidden or 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
|
✅ 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)' 'HEAD~1' HEAD clang/test/CodeGen/distributed-thin-lto/cs-irpgo.c clang/lib/CodeGen/BackendUtil.cpp clang/test/CodeGen/distributed-thin-lto/backend-skip.ll clang/test/CodeGen/distributed-thin-lto/basic.ll clang/test/CodeGen/distributed-thin-lto/cfi-devirt.ll clang/test/CodeGen/distributed-thin-lto/cfi.ll clang/test/CodeGen/distributed-thin-lto/newpm.ll clang/test/CodeGen/distributed-thin-lto/objc-contract-pass.ll clang/test/CodeGen/distributed-thin-lto/supports-hot-cold-new.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. |
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
clang/test/CodeGen/distributed-thin-lto/thinlto-distributed-backend-skip.ll
Outdated
Show resolved
Hide resolved
ellishg
reviewed
Mar 5, 2025
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?
There was a problem hiding this comment.
Because the test was moved down a directory, but the checked in sharded summary used as input to the test hasn't moved.
NuriAmari
force-pushed
the
nuriamari/dthin-lto-cs-irpgo
branch
from
4ef49b0 to
242defe
Compare
added 3 commits
June 6, 2025 09:12
clang/test/CodeGen includes many tests, some of which pretain to distributed thin-lto. While they have similar names, it is easier to find them all in a common directory.
When -fcs-profile-generate is used with a distributed compile without specifying a profile output path somehow, we fail the following assertion: https://github.com/llvm/llvm-project/blob/6041c745f32e8fd60ed24e29e7d919d8d1c87ca6/llvm/lib/Support/PGOOptions.cpp#L36 Avoid the assertion by choosing a default output path as other LTO mechanisms (ie. local thin-lto) do.
NuriAmari
force-pushed
the
nuriamari/dthin-lto-cs-irpgo
branch
from
242defe to
81e5a79
Compare
rorth
pushed a commit
to rorth/llvm-project
that referenced
this pull request
Jun 11, 2025
…thin-lto (llvm#129736) When using `-fcs-generate-profile` with distributed thin-lto in the same fashion we do for local thin-lto, we hit the following assertion: https://github.com/llvm/llvm-project/blob/6041c745f32e8fd60ed24e29e7d919d8d1c87ca6/llvm/lib/Support/PGOOptions.cpp#L36 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. --------- Co-authored-by: Nuri Amari <[email protected]>
DhruvSrivastavaX
pushed a commit
to DhruvSrivastavaX/lldb-for-aix
that referenced
this pull request
Jun 12, 2025
…thin-lto (llvm#129736) When using `-fcs-generate-profile` with distributed thin-lto in the same fashion we do for local thin-lto, we hit the following assertion: https://github.com/llvm/llvm-project/blob/6041c745f32e8fd60ed24e29e7d919d8d1c87ca6/llvm/lib/Support/PGOOptions.cpp#L36 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. --------- Co-authored-by: Nuri Amari <[email protected]>
tomtor
pushed a commit
to tomtor/llvm-project
that referenced
this pull request
Jun 14, 2025
…thin-lto (llvm#129736) When using `-fcs-generate-profile` with distributed thin-lto in the same fashion we do for local thin-lto, we hit the following assertion: https://github.com/llvm/llvm-project/blob/6041c745f32e8fd60ed24e29e7d919d8d1c87ca6/llvm/lib/Support/PGOOptions.cpp#L36 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. --------- Co-authored-by: Nuri Amari <[email protected]>
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.