Skip to content

[BOLT][NFC] Unset UseAssemblerInfoForParsing for emission #94778

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

aaupov
Copy link
Contributor

@aaupov aaupov commented Jun 7, 2024

Summary:
Use workaround for quadratic behavior inside
AttemptToFoldSymbolOffsetDifference called from BinaryEmitter::emitLSDA.

b06e736#commitcomment-142836456

Summary:
Use workaround for quadratic behavior inside
AttemptToFoldSymbolOffsetDifference called from BinaryEmitter::emitLSDA.

llvm@b06e736#commitcomment-142836456
@aaupov aaupov requested a review from MaskRay June 7, 2024 17:55
@llvmbot llvmbot added the BOLT label Jun 7, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 7, 2024

@llvm/pr-subscribers-bolt

Author: Amir Ayupov (aaupov)

Changes

Summary:
Use workaround for quadratic behavior inside
AttemptToFoldSymbolOffsetDifference called from BinaryEmitter::emitLSDA.

b06e736#commitcomment-142836456


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

1 Files Affected:

  • (modified) bolt/lib/Core/BinaryEmitter.cpp (+2)
diff --git a/bolt/lib/Core/BinaryEmitter.cpp b/bolt/lib/Core/BinaryEmitter.cpp
index 0b44acb0816f2..5793963f9b80d 100644
--- a/bolt/lib/Core/BinaryEmitter.cpp
+++ b/bolt/lib/Core/BinaryEmitter.cpp
@@ -194,6 +194,7 @@ class BinaryEmitter {
 
 void BinaryEmitter::emitAll(StringRef OrgSecPrefix) {
   Streamer.initSections(false, *BC.STI);
+  Streamer.setUseAssemblerInfoForParsing(false);
 
   if (opts::UpdateDebugSections && BC.isELF()) {
     // Force the emission of debug line info into allocatable section to ensure
@@ -226,6 +227,7 @@ void BinaryEmitter::emitAll(StringRef OrgSecPrefix) {
   // TODO Enable for Mach-O once BinaryContext::getDataSection supports it.
   if (BC.isELF())
     AddressMap::emit(Streamer, BC);
+  Streamer.setUseAssemblerInfoForParsing(true);
 }
 
 void BinaryEmitter::emitFunctions() {

@MaskRay
Copy link
Member

MaskRay commented Jun 7, 2024

Any idea why there is a quadratic behavior? Is the LayoutOrder cache not utilized? The following code should only be executed once for each section.

        for (MCFragment &F : *FA->getParent())
          F.setLayoutOrder(++LayoutOrder);

If you have any smaller example, I am happy to investigate by myself. My arbitrary picking one bold/test test doesn't find any repeated execution pattern.

@aaupov
Copy link
Contributor Author

aaupov commented Jun 7, 2024

Any idea why there is a quadratic behavior? Is the LayoutOrder cache not utilized? The following code should only be executed once for each section.

        for (MCFragment &F : *FA->getParent())
          F.setLayoutOrder(++LayoutOrder);

If you have any smaller example, I am happy to investigate by myself. My arbitrary picking one bold/test test doesn't find any repeated execution pattern.

The issue manifests only for large binaries with LSDA (C++ exceptions). RocksDB might be a suitable target. I can try to prepare the binary and add it to https://github.com/rafaelauler/bolt-tests

@MaskRay
Copy link
Member

MaskRay commented Jun 7, 2024

Any idea why there is a quadratic behavior? Is the LayoutOrder cache not utilized? The following code should only be executed once for each section.

        for (MCFragment &F : *FA->getParent())
          F.setLayoutOrder(++LayoutOrder);

If you have any smaller example, I am happy to investigate by myself. My arbitrary picking one bold/test test doesn't find any repeated execution pattern.

The issue manifests only for large binaries with LSDA (C++ exceptions). RocksDB might be a suitable target. I can try to prepare the binary and add it to rafaelauler/bolt-tests

Thanks! I guess you probably came to the wrong culprit b06e736 , which made me confused. The culprit for your use case might be the trigger #91082 .

LGTM. These getUseAssemblerInfoForParsing instances are tech debt, which I hope will be eliminated at some point...

@aaupov aaupov merged commit 7520d0c into llvm:main Jun 7, 2024
8 checks passed
@aaupov aaupov deleted the UseAssemblerInfoForParsing branch June 7, 2024 21:46
nekoshirro pushed a commit to nekoshirro/Alchemist-LLVM that referenced this pull request Jun 9, 2024
Summary:
Use workaround for quadratic behavior inside
AttemptToFoldSymbolOffsetDifference called from BinaryEmitter::emitLSDA.

llvm@b06e736#commitcomment-142836456
Signed-off-by: Hafidz Muzakky <[email protected]>
@HerrCai0907 HerrCai0907 mentioned this pull request Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants