[BOLT] Introduce BinaryFunction::canClone()#138771
Conversation
In some scenarios, we want to allow execution of the original function code together with its rewritten (optimized) copy. We used to limit such capability when the function uses C++ exceptions, since we cannot duplicate exception tables. There's more metadata that cannot be duplicated, and this PR adds more checks via the introduction of canClone() function that checks all such limitations.
maksfb
requested review from
aaupov,
ayermolo,
rafaelauler and
yota9
as code owners
|
@llvm/pr-subscribers-bolt Author: Maksim Panchenko (maksfb) ChangesIn some scenarios, we want to allow execution of the original function code together with its rewritten (optimized) copy. We used to limit such capability when the function uses C++ exceptions, since we cannot duplicate exception tables. There's more metadata that cannot be duplicated, and this PR adds more checks via the introduction of canClone() function that checks all such limitations. Full diff: https://github.com/llvm/llvm-project/pull/138771.diff 3 Files Affected:
diff --git a/bolt/include/bolt/Core/BinaryFunction.h b/bolt/include/bolt/Core/BinaryFunction.h
index a52998564ee1b..edeaa51481e6f 100644
--- a/bolt/include/bolt/Core/BinaryFunction.h
+++ b/bolt/include/bolt/Core/BinaryFunction.h
@@ -1375,6 +1375,11 @@ class BinaryFunction {
/// Return true if the function should not have associated symbol table entry.
bool isAnonymous() const { return IsAnonymous; }
+ /// Return true if we can allow the execution of the original body of the
+ /// function together with its rewritten copy. This means, e.g., that metadata
+ /// associated with the function can be duplicated/cloned.
+ bool canClone() const;
+
/// If this function was folded, return the function it was folded into.
BinaryFunction *getFoldedIntoFunction() const { return FoldedIntoFunction; }
diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp
index 9773e21aa7522..24972bac58409 100644
--- a/bolt/lib/Core/BinaryFunction.cpp
+++ b/bolt/lib/Core/BinaryFunction.cpp
@@ -3744,6 +3744,16 @@ void BinaryFunction::postProcessBranches() {
assert(validateCFG() && "invalid CFG");
}
+bool BinaryFunction::canClone() const {
+ // For instrumentation, we need to restrict the execution to the rewritten
+ // version of the function.
+ if (opts::Instrument)
+ return false;
+
+ // Check for the presence of metadata that cannot be duplicated.
+ return !hasEHRanges() && !hasSDTMarker() && !hasPseudoProbe() && !hasORC();
+}
+
MCSymbol *BinaryFunction::addEntryPointAtOffset(uint64_t Offset) {
assert(Offset && "cannot add primary entry point");
assert(CurrentState == State::Empty || CurrentState == State::Disassembled);
diff --git a/bolt/lib/Passes/PatchEntries.cpp b/bolt/lib/Passes/PatchEntries.cpp
index 4877e7dd8fdf3..8d07d163ef412 100644
--- a/bolt/lib/Passes/PatchEntries.cpp
+++ b/bolt/lib/Passes/PatchEntries.cpp
@@ -65,7 +65,7 @@ Error PatchEntries::runOnFunctions(BinaryContext &BC) {
continue;
// Check if we can skip patching the function.
- if (!opts::ForcePatch && !Function.hasEHRanges() &&
+ if (!opts::ForcePatch && Function.canClone() &&
Function.getSize() < PatchThreshold)
continue;
|
aaupov
left a comment
aaupov
left a comment
There was a problem hiding this comment.
LGTM with one question inline
| return false; | ||
|
|
||
| // Check for the presence of metadata that cannot be duplicated. | ||
| return !hasEHRanges() && !hasSDTMarker() && !hasPseudoProbe() && !hasORC(); |
There was a problem hiding this comment.
These checks seem quite restrictive (especially pseudo probes that are supposed to be in every function), what are the functions that can be cloned?
There was a problem hiding this comment.
I expect only tiny leaf function to be cloned, everything else should be patched.
There was a problem hiding this comment.
Note that I'm missing the practical details here.
What happens if we clone the pseudo-probes as well? Since they are uuids for blocks, would the cloned blocks also be identified as the original ones?
And if we don't clone them, are users expected to rely on pseudo-probes after a binary has been bolted?
| return false; | ||
|
|
||
| // Check for the presence of metadata that cannot be duplicated. | ||
| return !hasEHRanges() && !hasSDTMarker() && !hasPseudoProbe() && !hasORC(); |
There was a problem hiding this comment.
Note that I'm missing the practical details here.
What happens if we clone the pseudo-probes as well? Since they are uuids for blocks, would the cloned blocks also be identified as the original ones?
And if we don't clone them, are users expected to rely on pseudo-probes after a binary has been bolted?
|
As in previous review LG, but I fill it might be a bit over optimization, since it is very rare case and we're rely on conditions that might not cover all the cases in theory, maybe we should not have such thing at all... |
5 participants
In some scenarios, we want to allow execution of the original function code together with its rewritten (optimized) copy. We used to limit such capability when the function uses C++ exceptions, since we cannot duplicate exception tables. There's more metadata that cannot be duplicated, and this PR adds more checks via the introduction of canClone() function that checks all such limitations.