Page MenuHomePhabricator

[MLInliner] Factor out logging
ClosedPublic

Authored by mtrofin on Oct 2 2020, 8:30 PM.

Details

Summary

Factored out the logging facility, to allow its reuse outside the
inliner.

Diff Detail

Event Timeline

mtrofin created this revision.Oct 2 2020, 8:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2020, 8:30 PM
mtrofin requested review of this revision.Oct 2 2020, 8:30 PM
yundiqian added inline comments.Oct 5 2020, 10:56 AM
llvm/include/llvm/Analysis/Utils/TFUtils.h
106–120

add the documentation about calling print() after the trace is finished

llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp
116–122

initialize them as 0, -1, -1?

350

to confirm, it will still not have delta_size field? not a default value 0?

372–373

It seems that we don't need DefaultDecisionPos and DecisionPos by having logTensorValue(CurrentFeature++, &Event....) cause you are making assumptions anyway?

llvm/lib/Analysis/TFUtils.cpp
403–407

shall we also add a check here making sure all features are of equal length? we have no check about Logger is used in its intended way.

mtrofin updated this revision to Diff 296245.Oct 5 2020, 11:31 AM
mtrofin marked 5 inline comments as done.

feedback

llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp
116–122

OutputCount is at least 1, actually. Not sure what setting Pos values to max_uint64 would do - I guess it'd be checking we set them. OK.

350

It's up to the user. In this case, it won't. If the user needs to output a reward field, they can, and set it to 0 (except the last one) if it's for a final reward algo.

My thinking was that, since in this case we insert the final reward after compilation, when we measure the size, then maybe it'd be easier not to have the field in the first place.

Let me know if it'd be easier to always have the field there instead.

372–373

Either or. Let me add an assertion here that we got at the right place - that's what I wanted to use them for, really.

llvm/lib/Analysis/TFUtils.cpp
403–407

That'd be an assert, not a runtime check, because it'd be a bug; it'd also be detectable by code further below failing. In other words, should be detected by testing.

gjain requested changes to this revision.Oct 5 2020, 4:15 PM

Very minor comments...

llvm/include/llvm/Analysis/Utils/TFUtils.h
129

Could you explain why the IncludeReward flag is necessary? What is the harm in calling logReward? Is there a alternative setup where we can avoid having this flag (perhaps 2 different classes)?

llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp
77

Do you mean to set this to 0?

81

ditto

llvm/lib/Analysis/TFUtils.cpp
110

Indentation looks off here did you forget a closing curly from above?

This revision now requires changes to proceed.Oct 5 2020, 4:15 PM
mtrofin updated this revision to Diff 296329.Oct 5 2020, 4:40 PM
mtrofin marked 4 inline comments as done.

feedback

llvm/include/llvm/Analysis/Utils/TFUtils.h
129

I wouldn't subclass just on this aspect. To answer the question, the distinction is soft - in the inliner for size case, the reward won't be computed during the runtime of the compiler, so *maybe* it's easier to just add the whole field in post-processing. See my reply to Yundi's comment at DevelopmentModeInlineAdvisor.cpp:350

llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp
81

yup, thanks!

llvm/lib/Analysis/TFUtils.cpp
110

there's no curly, this is correct. I'll curly the llvm_unreachable, though, because I can see how their absence may cause confusion.

yundiqian accepted this revision.Oct 5 2020, 5:36 PM
yundiqian added inline comments.
llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp
350

yeah, I agree, not having it makes sense

llvm/lib/Analysis/TFUtils.cpp
403–407

ok, it's fine if this is covered by test

This revision was not accepted when it landed; it landed in state Needs Review.Oct 5 2020, 6:09 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.