This is an archive of the discontinued LLVM Phabricator instance.

Filtering IR printing for print-after-all/print-before-all
ClosedPublic

Authored by weimingz on Dec 24 2015, 12:19 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

weimingz updated this revision to Diff 43614.Dec 24 2015, 12:19 PM
weimingz retitled this revision from to [RFC] Selectively print functions for print-after-all/print-before-all.
weimingz updated this object.
weimingz set the repository for this revision to rL LLVM.
weimingz added a subscriber: llvm-commits.
weimingz retitled this revision from [RFC] Selectively print functions for print-after-all/print-before-all to Filtering IR printing for print-after-all/print-before-all.Jan 4 2016, 4:13 PM
weimingz updated this object.

Thanks for working on this, I thought about this multiple times, but never got to implement it. Some comments below.

include/llvm/Pass.h
372

Document

lib/Analysis/LoopPass.cpp
45–49

What if we don't have a header? When does it happen? Can this inhibit the print of a loop even when -print-funcs isn't supplied?
Else, could it be an assert instead?

lib/CodeGen/MachineFunctionPrinterPass.cpp
48

Early return is preferred (same for all the similar cases).

lib/IR/IRPrintingPasses.cpp
34

What about having the else here looping over the module and finding the functions selected to be printed and print them?

lib/IR/LegacyPassManager.cpp
514

Not sure if this is the right place to do that, what about a static variable local to isFunctionInPrintList(), it would be initialized on the first call to the function.

Agree with Mehdi, this will be very useful. Thanks!

lib/Analysis/CallGraphSCCPass.cpp
616

Combine above two ifs into a single "if (A && B)".

lib/IR/IRPrintingPasses.cpp
34

Agree here with Mehdi. Rather than have an all-or-nothing printing of the module under a special "*" string, it would be better to walk the module and print just the functions in the list. Otherwise to get any printing before/after module passes you run into the same issue with huge amounts of output for large modules. This would also be analogous to what you are doing for SCC passes (printing just the functions in the SCC that are in the list).

lib/IR/LegacyPassManager.cpp
90

Remove the "*" special case as described earlier.

weimingz marked 4 inline comments as done.Jan 5 2016, 12:31 PM
weimingz added inline comments.
lib/Analysis/CallGraphSCCPass.cpp
616

If we combine them, then for non-null function but not in our filter list, it will output "Printing <null> Function"

lib/Analysis/LoopPass.cpp
45–49

normally we wouldn't hit this.
But since there could be null blocks (see LoopInfo.cpp PrinterLoopPass::run() ), I added a check. If the code is broken, it should be caught by other passes. :D

lib/IR/IRPrintingPasses.cpp
34

Agree. I will add "*" to the list if we don't specify anything to filter. So by testing "*", we can tell if the Option is empty or not.
For SCC, it already has a loop.

weimingz updated this revision to Diff 44046.Jan 5 2016, 12:35 PM
weimingz updated this object.
mehdi_amini added inline comments.Jan 5 2016, 12:48 PM
lib/Analysis/LoopPass.cpp
45–50

Not clear to me: either there is a valid use case where PrintLoopPassWrapper::runOnLoop() is called with getHeader() returning null or not, can you clarify?

In the absence of such valid case, please turn this into an assert or just remove the check.

lib/IR/LegacyPassManager.cpp
128

Is it possible to write it:

static std::set<std::string> PrintFuncNames(PrintFuncsList.begin(), PrintFuncsList.end());

?

Also, why not an unordered_set?

131

I'm not sure I see the need for the "*" in the set? The empty case returning true seems enough to me to provide exactly the same behavior.

weimingz marked an inline comment as not done.Jan 5 2016, 1:45 PM
weimingz added inline comments.
lib/Analysis/LoopPass.cpp
45–50

Could be a valid case. Below is PrintLoopPass::run()
PrintLoopPass::run(Loop &L) {

 ..
for (auto *Block : L.blocks())
  if (Block)
    Block->print(OS);
  else
    OS << "Printing <null> block";

..
}

lib/IR/LegacyPassManager.cpp
128

Yes, we can write in one line.

yes, I meant to use unordered_set, but forgot the change. Thanks!

131

Need that because in Module print, we need to test if the filter-list is empty or not. (An empty list means the whole module will be printed).

isFunctionInPrintList() only checks if the argument string is in the list or not. Alternatively, we can let it return a pair of bools, one indicates if the list is empty or not, the other tells if the arg is in list or not. But this is not intuitive.

weimingz updated this revision to Diff 44057.Jan 5 2016, 1:55 PM
tejohnson added inline comments.Jan 5 2016, 1:59 PM
lib/Analysis/CallGraphSCCPass.cpp
616

Oh I see, we don't want to print anything when the function is suppressed. Nevermind my suggestion then.

lib/IR/LegacyPassManager.cpp
131

How about just return true when PrintFuncNames is empty? I see you are currently using that condition to guard initialization. You can either add a different static bool to indicate that it was already initialized, or key off of PrintFuncsList being non-empty to guard the initialization.

Then you don't need the special case in PrintModulePass::run either.

weimingz added inline comments.Jan 5 2016, 2:13 PM
lib/IR/LegacyPassManager.cpp
125

Module printing is different. It not just prints each functions but also others like meta, GV, triple, etc. By default (not specify anything on the filter list), it should print all those stuff.

So we need two functionalities: 1) tell if some function is in list or not; 2) tell if we specified anything to the list.

mehdi_amini added inline comments.Jan 5 2016, 4:14 PM
lib/Analysis/LoopPass.cpp
45–50

I see, but it means that here we shouldn't check for the header but get the first valid block to get the function, something like:

auto FirstValidBlockInLoop = std::find_if(L.blocks().start(), L.blocks().end(), [] (BasicBlock *Block) { return Block; });
if (FirstValidBlockInLoop != L.blocks().end() && 
    isFunctionInPrintList(FirstValidBlockInLoop->getParent()->getName()))
      P.run(*L);
lib/IR/LegacyPassManager.cpp
125

You don't need to add "*" to the set to have the empty case working since in the empty case true is returned for anything.

mehdi_amini added inline comments.Jan 5 2016, 4:17 PM
lib/IR/LegacyPassManager.cpp
125

To be more clear, I don't see why this wouldn't work:

bool llvm::isFunctionInPrintList(StringRef FunctionName) {
  static std::unordered_set<std::string> PrintFuncNames(PrintFuncsList.begin(),
                                                        PrintFuncsList.end());

  if (PrintFuncNames.empty())
    return true;

  return PrintFuncNames.count(FunctionName);
}
mehdi_amini added inline comments.Jan 5 2016, 4:18 PM
lib/IR/LegacyPassManager.cpp
90

Just thought about this, we already have something to filter basic blocks in the DAG output, and the option is named filter-view-dags, so I feel this option should also start with filter for consistency.

weimingz added inline comments.Jan 5 2016, 5:02 PM
lib/IR/LegacyPassManager.cpp
90

Agree. Will change it to -filter-print-funcs

125

In Module print, we have 2 cases to handle now:

  1. the default case / the filter list is empty(): M.print(OS, nullptr, ShouldPreserveUseListOrder); // this will dump all module info
  2. the list is non empty. No GV, meta info will be output and only functions in the filter list will be printed.

The code you show either dumps all funcs if list is empty or dump filtered funcs. But it doesn't differentiate case 1 and case 2.

mehdi_amini added inline comments.Jan 5 2016, 5:25 PM
lib/IR/LegacyPassManager.cpp
125

What about:

bool llvm::isFunctionInPrintList(StringRef FunctionName) {
  static std::unordered_set<std::string> PrintFuncNames(PrintFuncsList.begin(),
                                                        PrintFuncsList.end());

  if (PrintFuncNames.empty())
    return true;

  return PrintFuncNames.count(FunctionName) || PrintFuncNames.count("*");
}
mehdi_amini added inline comments.Jan 5 2016, 5:27 PM
lib/IR/LegacyPassManager.cpp
125

Nevermind forget my last comment, I still don't see any use for that. I still don't get your use case.

Can you provide an input where it differs?

weimingz added inline comments.Jan 5 2016, 5:48 PM
lib/IR/LegacyPassManager.cpp
125

For example, when the filter-list is empty. the whole module info is expected.
So in lib/IR/IRPrintingPasses.cpp, we should call

M.print(...);

The loop below should be run only when the filter list is not empty:

for(const auto &F : M.functions())
 if (llvm::isFunctionInPrintList(F.getName()))  // Although it can return T if filter list is empty, but we don't want to just print function level IRs
     F.print(OS);
mehdi_amini added inline comments.Jan 5 2016, 5:57 PM
lib/IR/LegacyPassManager.cpp
125

I understand that, I just don't see why it does not work with the implementation I gave you.

Can we focus on isFunctionInPrintList() can you give a case where the implementation I gave you does not return true or false as expected?

weimingz updated this revision to Diff 44076.Jan 5 2016, 6:26 PM

LOL. I misunderstood what you said.
Now I got it. Thanks, Mehdi!

mehdi_amini accepted this revision.Jan 5 2016, 6:28 PM
mehdi_amini edited edge metadata.

I'm glad we sorted out.
LGTM, thanks.

This revision is now accepted and ready to land.Jan 5 2016, 6:28 PM
weimingz closed this revision.Jan 6 2016, 10:23 AM
weimingz updated this revision to Diff 44164.Jan 6 2016, 2:50 PM
weimingz edited edge metadata.

Remove an extra "\n" output that causes some lit test fail.