This is an archive of the discontinued LLVM Phabricator instance.

New OptimizationRemarkEmitter pass for MIR
ClosedPublic

Authored by anemet on Jan 22 2017, 9:51 PM.

Details

Summary

This allows MIR passes to emit optimization remarks with the same level
of functionality that is available to IR passes.

We need a MIR-specific analysis pass because this pass uses MachineBFI while ORE uses BFI.

It also hooks up the greedy register allocator to report spills. This
allows for interesting use cases like increasing
interleaving on a loop until spilling of registers is observed.

I still need to experiment whether reporting every spill scales but this
demonstrates for now that the functionality works from llc
using -pass-remarks*=<pass>.

Diff Detail

Repository
rL LLVM

Event Timeline

anemet created this revision.Jan 22 2017, 9:51 PM
anemet edited the summary of this revision. (Show Details)Jan 23 2017, 8:31 AM
MatzeB edited edge metadata.Jan 23 2017, 10:39 AM

I am a bit confused by the isEnabled() code. Is this meant as a common tool for frontends to configure filtering? I couldn't find any user in llvm before this patch, maybe I missed something? Shouldn't opt and clang have similar lines like the 3 lines you added to llc.cpp?

include/llvm/CodeGen/MachineOptimizationRemarkEmitter.h
10–12 ↗(On Diff #85321)

Use /// \file ...

27 ↗(On Diff #85321)

by machine passes.

31 ↗(On Diff #85321)

Side remarks that we should not fix in this commit (because it's the same for other classes):

  • The base class is specifically designed to use an int for the diagnostic kind to stay extensible for plugins and then all the subclasses just go back to enum DiagnosticKind.
  • DebugLoc is basically a pointer with special semantics so it is simpler and more efficient to pass it by value instead of const DebugLoc&.
43 ↗(On Diff #85321)

Could be const

127–128 ↗(On Diff #85321)

Use reference for MF and MBFI if they cannot be nullptr.

131 ↗(On Diff #85321)

/// Emits an optimization remark ?

142–143 ↗(On Diff #85321)

Wouldn't a user of -Rpass not want false positives eliminated as well? I keep wondering why not just using isEnabled() to decide whether to do extra work or not.

169 ↗(On Diff #85321)

Wouldn't you expect the big majority of uses to be as an analysis pass? So maybe this could be merged with MachineOptimizationRemarkEmitter to keep things simpler? (We can always revert this decision when we find cases where we want a non-pass emitter).

179 ↗(On Diff #85321)

Can be const

lib/CodeGen/InlineSpiller.cpp
181 ↗(On Diff #85321)

Why not `pass.getAnalysis<MachineOptimizationRemarkEmitter>() instead of passing around the instance?

lib/CodeGen/RegAllocGreedy.cpp
2639–2640 ↗(On Diff #85321)

I assume we do not need to create a new spiller instance each time when you change the spiller to do the getAnalysis<> (see above).

Thinking some more about what is reported here: Reporting every single spill will produce a lot of data and most probably force optviewer to summarize/aggregate the information anyway to be any useful. My expectation would be that anything finer grained than per-loop is probably unnecessary, so:

Maybe a better approach would be to look whether reporting is enabled and if so walk the machine loops after greedy is finished. And then count the spill/reload instructions for each loop and only report that as a remark. A technique as used in AsmPrinter.cpp:emitComments() should be good enough to identify an instruction as performing a spill/reload.

I am a bit confused by the isEnabled() code. Is this meant as a common tool for frontends to configure filtering? I couldn't find any user in llvm before this patch, maybe I missed something? Shouldn't opt and clang have similar lines like the 3 lines you added to llc.cpp?

So backend users can use the default diagnostic handler (like opt) or define their own (like llc and clang). The default handler is in LLVMContext::diagnose which has similar code via isDiagnosticEnabled.

include/llvm/CodeGen/MachineOptimizationRemarkEmitter.h
142–143 ↗(On Diff #85321)

I think that the comments are misleading. The idea of this flag is not to reduce the number of false positives but to be able to report cases where the false positive rate is expected to be high without some further analysis.

In other words if this flag is off (i.e. -Rpass) we won't report these cases at all.

Let me fix the comment in a separate commit.

169 ↗(On Diff #85321)

The other reason why this got split for LLVM-IR ORE was the new pass manager where you have now "two" passes (legacy, new) producing ORE.

I think I would prefer to keep this separation.

lib/CodeGen/InlineSpiller.cpp
181 ↗(On Diff #85321)

Because the InlineSpiller is used from other allocators as well. I'll add a comment that explains this.

MatzeB added inline comments.Jan 23 2017, 1:03 PM
include/llvm/CodeGen/MachineOptimizationRemarkEmitter.h
142–143 ↗(On Diff #85321)

Ah that makes sense. I guess even the term "false positive" feels a bit out of place for information like where/how much was spilled. As those aren't immediately actionable things but rather extra information / statistics (at least when viewed in isolation without having something to compare against).

169 ↗(On Diff #85321)

Ah I didn't think about the new pass manager, makes sense.

lib/CodeGen/InlineSpiller.cpp
181 ↗(On Diff #85321)

Sure it's used by multiple allocators, but I don't see why this would prohibit the use of getAnalysis<> here.

anemet updated this revision to Diff 85668.Jan 24 2017, 5:28 PM
anemet marked 7 inline comments as done.

The main change is that we now report spills & reloads per loop as suggested
by Matthias.

For this I had to somehow find the source line info for a MachineLoop (see
MachineLoop::getStartLoc()). This is a dumbed-down version of the LLVM IR
variant which uses LoopID. We can explore later if we can somehow find our
way back to the IR LoopID.

anemet added inline comments.Jan 24 2017, 5:29 PM
include/llvm/CodeGen/MachineOptimizationRemarkEmitter.h
127–128 ↗(On Diff #85321)

MF is done. MBFI can be null.

179 ↗(On Diff #85321)

No, we need to call emit through this reference.

MatzeB accepted this revision.Jan 25 2017, 11:05 AM

LGTM, the issues mentioned can be fixed without further review.

include/llvm/CodeGen/MachineOptimizationRemarkEmitter.h
9 ↗(On Diff #85668)

Needs three slashes.

154 ↗(On Diff #85668)

Use a reference for MBB parameter as it cannot be nullptr.

179 ↗(On Diff #85321)

getORE() const should still be possible (const MachineOptimizationRemarkEmitter& is not possible of course)

lib/CodeGen/MachineOptimizationRemarkEmitter.cpp
9 ↗(On Diff #85668)

/// \file ...

34–35 ↗(On Diff #85668)

Could move the declaration into the if

lib/CodeGen/RegAllocGreedy.cpp
430 ↗(On Diff #85668)

Would it make sense to add a if (!OptimizationRemark::isEnabled(DEBUG_TYPE)) return; here?

438 ↗(On Diff #85668)

Shouldn't this do Reloads = 0; FoldedReloads = 0; ... in each loop iteration as we shouldn't add the values of an earlier loop to a later loop (we just want it from inner loops to outer loops I assume).

This revision is now accepted and ready to land.Jan 25 2017, 11:05 AM
anemet marked 3 inline comments as done.Jan 25 2017, 2:15 PM
anemet added inline comments.
lib/CodeGen/MachineOptimizationRemarkEmitter.cpp
34–35 ↗(On Diff #85668)

No, they control-dependent :)

lib/CodeGen/RegAllocGreedy.cpp
430 ↗(On Diff #85668)

No, -fsave-optimization-record should get this regardless of Enabled. Filtering is on the consumer side.

438 ↗(On Diff #85668)

What?! You can't count for a single loop and for all contained loops in the same variable?! Thanks for noticing!

Also extended the test to cover for this.

This revision was automatically updated to reflect the committed changes.
anemet marked an inline comment as done.