HomePhabricator

Revert "Extend CFGPrinter and CallPrinter with Heat Colors"

Description

Revert "Extend CFGPrinter and CallPrinter with Heat Colors"

This reverts r335996 which broke graph printing in Polly.

Details

Committed
sfertileJun 29 2018, 10:48 AM
Parents
rL335999: AMDGPU: Don't use struct type for argument layout
Branches
Unknown
Tags
Unknown

Event Timeline

rcorcs added a subscriber: rcorcs.Jul 10 2018, 5:50 AM

Hi,

I've fixed the graph printing in Polly.

Should I create a separate patch for Polly?

Thanks.

Hi,

I've fixed the graph printing in Polly.

Should I create a separate patch for Polly?

Thanks.

Awesome! It will need to be posted and reviewed as a separate patch by people familiar with Polly. Please subscribe me to it as well.

@rcorcs

I've been looking into the sanitizer build-bot failure and the address sanitizer detects a leak in the old-pass manager. One of my team members encountered this same problem before, where a seemingly unrelated change caused the address sanitize to detect a leak in the old pass manger. I am hoping to get a chance to look at it closer tonight to see if there is a real problem or not.

 /home/sean/llvm-sanitize/bin/opt  /home/sean/DevSrc/llvm/test/Other/2007-06-05-PassID.ll -analyze -dot-cfg-only  
Writing 'cfg.foo.dot'...
Printing analysis 'Print CFG of function to 'dot' file (with no function bodies)':

=================================================================
==13703==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 408 byte(s) in 1 object(s) allocated from:
    #0 0xd83b32  (/home/sean/llvm-sanitize/bin/opt+0xd83b32) /home/sean/DevSrc/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc,   line 106
    #1 0x29ecb6f  (/home/sean/llvm-sanitize/bin/opt+0x29ecb6f) /home/sean/DevSrc/llvm/include/llvm/PassSupport.h, line  77
    #2 0x3900f5c  (/home/sean/llvm-sanitize/bin/opt+0x3900f5c) /home/sean/DevSrc/llvm/lib/IR/LegacyPassManager.cpp, line 1095
    #3 0x38fd281  (/home/sean/llvm-sanitize/bin/opt+0x38fd281) /home/sean/DevSrc/llvm/lib/IR/LegacyPassManager.cpp, line 763.
    #4 0xde47a5  (/home/sean/llvm-sanitize/bin/opt+0xde47a5) /home/sean/DevSrc/llvm/tools/opt/opt.cpp, line 301
    #5 0xde1b38  (/home/sean/llvm-sanitize/bin/opt+0xde1b38)
    #6 0x7fceca836b34  (/lib64/libc.so.6+0x21b34)


SUMMARY: AddressSanitizer: 408 byte(s) leaked in 1 allocation(s).

@rcorcs i t looks like simply changing the order of the addRequired to put BPI before BFI fixes the reported leak:

diff --git a/lib/Analysis/CFGPrinter.cpp b/lib/Analysis/CFGPrinter.cpp
index 10be4e4..396f352 100644
--- a/lib/Analysis/CFGPrinter.cpp
+++ b/lib/Analysis/CFGPrinter.cpp
@@ -228,8 +228,8 @@ namespace {
 
     void getAnalysisUsage(AnalysisUsage &AU) const override {
       ModulePass::getAnalysisUsage(AU);
-      AU.addRequired<BlockFrequencyInfoWrapperPass>();
       AU.addRequired<BranchProbabilityInfoWrapperPass>();
+      AU.addRequired<BlockFrequencyInfoWrapperPass>();
       AU.setPreservesAll();
     }
 
@@ -275,8 +275,8 @@ namespace {
 
     void getAnalysisUsage(AnalysisUsage &AU) const override {
       ModulePass::getAnalysisUsage(AU);
-      AU.addRequired<BlockFrequencyInfoWrapperPass>();
       AU.addRequired<BranchProbabilityInfoWrapperPass>();
+      AU.addRequired<BlockFrequencyInfoWrapperPass>();
       AU.setPreservesAll();
     }

I was testing this out on Power though since I can build much faster there. I'll see if this fixes the leak on X86 as well ....

Tested the above change out on my laptop and if it fixes the leak failure. I'm going to see if I can reproduce the problem in another pass and open a bugzilla if I can reproduce it.