Page MenuHomePhabricator

[LoopFusion] Extend use of OptimizationRemarkEmitter
ClosedPublic

Authored by kbarton on Jun 26 2019, 2:21 PM.

Details

Summary

This patch extends the use of the OptimizationRemarkEmitter to provide
information about loops that are not fused, and loops that are not eligible for
fusion. In particular, it uses the OptimizationRemarkAnalysis to identify loops
that are not eligible for fusion and the OptimizationRemarkMissed to identify
loops that cannot be fused.

It also reuses the statistics to provide the messages used in the
OptimizationRemarks. This provides common message strings between the
optimization remarks and the statistics.

I would like feedback on this approach, in general. If people are OK with this,
I will flesh out additional remarks in subsequent commits.

Event Timeline

kbarton created this revision.Jun 26 2019, 2:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2019, 2:21 PM

I went over the whole patch, except a small detail this all looks perfectly fine to me. Given that still want to "flesh out details" I will wait with a final review but I do not expect a problem.

llvm/lib/Transforms/Scalar/LoopFuse.cpp
1176

I would somehow prefer if the two functions above were merged or at least one uses the other. The ORE part is almost identical and we could, probably should, increment the relevant statistic in reportLoopFusion as well.

kbarton marked an inline comment as done.Jun 27 2019, 9:56 AM
kbarton added inline comments.
llvm/lib/Transforms/Scalar/LoopFuse.cpp
1176

I agree passing the stat into reportLoopFusion makes sense. I'll make that change.

I don't quite understand the point about commoning these two functions into a single function though. They fundamentally do two different things: for successful fusion we emit to OptimizationRemark; for unsuccessful fusion we emit to OptimizationRemarkMissed.

So, in order to common these up there needs to be a check to figure out which OptimizationRemark class to use. At that point, the only commonality that you get is the increment of Stat.

Am I missing something?

Whitney added inline comments.Jun 27 2019, 10:40 AM
jdoerfert added inline comments.Jun 27 2019, 2:37 PM
llvm/lib/Transforms/Scalar/LoopFuse.cpp
1176

They fundamentally do two different things:

I think they do the same, remark emission and bookkeeping.

So, in order to common these up there needs to be a check to figure out which OptimizationRemark class to use. At that point, the only commonality that you get is the increment of Stat.

We know at the call site, right?

template<typename RemarkKind> reportFusion(...) {
 ...
 ORE.emit(RemarkKind(DEBUG_TYPE, Stat.getName(), FC0.L->getStartLoc(), FC0.Preheader)
  << ...
}


reportFusion<OptimizationRemark>(....);

reportFusion<OptimizationRemarkMissed>(....);
etiotto requested changes to this revision.Jun 28 2019, 7:18 AM

This looks pretty good to me. I just added some minor inline comments.

llvm/lib/Transforms/Scalar/LoopFuse.cpp
82

Nit: InvalidTripCount --> UnknownTripCount

1148

Can you please add a comment similar to the comment for 'reportNoFusion'. Also what do you think about adding a statistic to count the number of successful fusion opportunities?

1149

assert FC[0|1]->Preheader != nullptr ?

This revision now requires changes to proceed.Jun 28 2019, 7:18 AM
fhahn added a subscriber: fhahn.Jun 28 2019, 7:23 AM
fhahn added inline comments.
llvm/test/Transforms/LoopFusion/diagnostics_analysis.ll
4

Can this be dropped?

16

is the tbaa metadata required for the test? I think the debug metadata could also be thinned out a bit. Same for the other test.

Meinersbur added inline comments.Jul 9 2019, 4:32 PM
llvm/lib/Transforms/Scalar/LoopFuse.cpp
236
kbarton updated this revision to Diff 209938.Jul 15 2019, 1:11 PM

Address the following review comments:

  1. Create a function template to report successful and missed loop fusion opportunities.
  2. Cleanup debug metadata in the test cases.
  3. Address other minor comments (pre-increment of stats, function name, missing asserts).
kbarton marked 8 inline comments as done.Jul 15 2019, 1:14 PM
kbarton updated this revision to Diff 209939.Jul 15 2019, 1:19 PM
  • Rename InvalidTripCount to UnknownTripCount as per review comments.
kbarton marked an inline comment as done.Jul 15 2019, 1:21 PM

@etiotto Any additional comments?

etiotto accepted this revision.Tue, Jul 30, 6:35 AM

LGTM. I found a typo, you can fix it while committing.

llvm/lib/Transforms/Scalar/LoopFuse.cpp
1164

typo: Expectingf --> Expecting

This revision is now accepted and ready to land.Tue, Jul 30, 6:35 AM
kbarton closed this revision.Tue, Jul 30, 9:00 AM