Skip to content
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

[StandardInstrumentation] Annotate loops with the function name #90756

Merged
merged 1 commit into from
May 3, 2024

Conversation

annamthomas
Copy link
Contributor

When analyzing pass debug output it is helpful to have the function name along with the loop name.

@annamthomas
Copy link
Contributor Author

I've not updated all the failing tests check lines, want to know this is okay before going forward.

@llvmbot
Copy link
Collaborator

llvmbot commented May 1, 2024

@llvm/pr-subscribers-llvm-transforms

Author: None (annamthomas)

Changes

When analyzing pass debug output it is helpful to have the function name along with the loop name.


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

3 Files Affected:

  • (modified) llvm/lib/Passes/StandardInstrumentations.cpp (+2-1)
  • (modified) llvm/test/Transforms/LoopPredication/invalidate-analyses.ll (+2-2)
  • (modified) llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-markloopasdeleted.ll (+2-2)
diff --git a/llvm/lib/Passes/StandardInstrumentations.cpp b/llvm/lib/Passes/StandardInstrumentations.cpp
index 63490c83e85f05..990ace8b04f87b 100644
--- a/llvm/lib/Passes/StandardInstrumentations.cpp
+++ b/llvm/lib/Passes/StandardInstrumentations.cpp
@@ -245,7 +245,8 @@ std::string getIRName(Any IR) {
     return C->getName();
 
   if (const auto *L = unwrapIR<Loop>(IR))
-    return L->getName().str();
+    return "Loop: [ " + L->getName().str() + " ] In Function: [ " +
+           L->getHeader()->getParent()->getName().str() + " ]";
 
   if (const auto *MF = unwrapIR<MachineFunction>(IR))
     return MF->getName().str();
diff --git a/llvm/test/Transforms/LoopPredication/invalidate-analyses.ll b/llvm/test/Transforms/LoopPredication/invalidate-analyses.ll
index 7afacd564939b5..b3eb345e1ba6cd 100644
--- a/llvm/test/Transforms/LoopPredication/invalidate-analyses.ll
+++ b/llvm/test/Transforms/LoopPredication/invalidate-analyses.ll
@@ -5,10 +5,10 @@
 ;       please update this test some other analysis that isn't preserved.
 
 ; CHECK: Running analysis: LazyValueAnalysis on drop_a_wc_and_leave_early
-; CHECK: Running pass: LoopPredicationPass on loop
+; CHECK: Running pass: LoopPredicationPass on Loop: [ loop ] In Function: [ drop_a_wc_and_leave_early ] 
 ; CHECK: Invalidating analysis: LazyValueAnalysis on drop_a_wc_and_leave_early
 ; CHECK: Running analysis: LazyValueAnalysis on drop_a_wc_and_leave
-; CHECK: Running pass: LoopPredicationPass on loop
+; CHECK: Running pass: LoopPredicationPass on Loop: [ loop ] In Function: [ drop_a_wc_and_leave ] 
 ; CHECK: Invalidating analysis: LazyValueAnalysis on drop_a_wc_and_leave
 
 
diff --git a/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-markloopasdeleted.ll b/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-markloopasdeleted.ll
index c8e1291b9cd55c..e47b76f58833b3 100644
--- a/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-markloopasdeleted.ll
+++ b/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-markloopasdeleted.ll
@@ -17,9 +17,9 @@
 ; SimpleLoopUnswitch not marking the Loop as removed, so we missed clearing
 ; the analysis caches.
 ;
-; CHECK: Running pass: SimpleLoopUnswitchPass on loop_begin
+; CHECK: Running pass: SimpleLoopUnswitchPass on Loop: [ loop_begin ] In Function: [ test6 ]
 ; CHECK-NEXT: Running analysis: OuterAnalysisManagerProxy
-; CHECK-NEXT: Clearing all analysis results for: loop_a_inner
+; CHECK-NEXT: Clearing all analysis results for: Loop [ loop_a_inner ] In Function: [ test6 ]
 
 
 ; When running loop-distribute the second time we can see that loop_a_inner

@aeubanks
Copy link
Contributor

aeubanks commented May 1, 2024

makes sense to me, although bikeshedding a bit, I'd prefer
Running pass: SimpleLoopUnswitchPass on loop %loop_begin in function test6

happy to hear any other tweaks to what actually gets printed

@annamthomas
Copy link
Contributor Author

makes sense to me, although bikeshedding a bit, I'd prefer
Running pass: SimpleLoopUnswitchPass on loop %loop_begin in function test6

Yes, I can change to that. I'll wait a day to see if anyone has other preferences.

@jamieschmeiser
Copy link
Contributor

jamieschmeiser commented May 1, 2024

I like the idea of making the loop easier to identify. Have you considered demangling the function name also, or is that already handled in getName()? However, that may make filtering difficult... what about printing both the mangled and demangled name if it is different?

This is a suggestion; I wouldn't block this change for demangling.

@nikic
Copy link
Contributor

nikic commented May 2, 2024

makes sense to me, although bikeshedding a bit, I'd prefer Running pass: SimpleLoopUnswitchPass on loop %loop_begin in function test6

+1

I like the idea of making the loop easier to identify. Have you considered demangling the function name also, or is that already handled in getName()? However, that may make filtering difficult... what about printing both the mangled and demangled name if it is different?

I would not find demangling helpful here -- if I'm looking at debug logs, I want to line them up with IR, not with source.

@jamieschmeiser
Copy link
Contributor

I would not find demangling helpful here -- if I'm looking at debug logs, I want to line them up with IR, not with source.

I guess it depends on the situation as to whether demangling would be useful. I find demangled names easier to understand but the filtering mechanisms work with mangled names. This is why I suggested both but that may be too much info on the line. At the risk of over-designing something simple, perhaps demangling could be controlled by a hidden option?

Demangled or not, adding the function name is a good step.

@annamthomas
Copy link
Contributor Author

Suggestion about formatting noted.
For now, I'm just going to be adding the function name to progress ahead with this (and general helps with the use cases I'm after).

Copy link
Contributor

@aeubanks aeubanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with comment addressed

llvm/test/Transforms/LoopUnroll/revisit.ll Outdated Show resolved Hide resolved
When analyzing pass debug output it is helpful to have the function name
along with the loop name.
@annamthomas annamthomas merged commit 46c2d93 into llvm:main May 3, 2024
3 of 4 checks passed
@jamieschmeiser
Copy link
Contributor

I had lost track of #87626 but the memory of it was haunting me during this review. It hasn't landed yet but it may be something you want to look at.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants