This is an archive of the discontinued LLVM Phabricator instance.

[NFC][MLInliner] Refactor logging implementation
ClosedPublic

Authored by mtrofin on Aug 6 2020, 11:03 AM.

Details

Summary

This prepares it for logging externally-specified outputs.

Diff Detail

Event Timeline

mtrofin created this revision.Aug 6 2020, 11:03 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2020, 11:03 AM
mtrofin requested review of this revision.Aug 6 2020, 11:03 AM
yundiqian added inline comments.Aug 7 2020, 1:29 AM
llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp
107

when we will be using this?

123–138

is it possible to template this as what you did in last two diffs?

162

should Effects also becomes int64_t if Decisions are int64_t?

382–384

shall we log Effect instead of Decision?

mtrofin marked 3 inline comments as done.Aug 7 2020, 9:33 AM
mtrofin added inline comments.
llvm/lib/Analysis/DevelopmentModeInlineAdvisor.cpp
107

This is what typed cases call - e.g. when we log the feature tensors. We know their type at compile time. It saves us from typecasting all the time the data buffer.

123–138

don't think it'll add much value, there are just the 3 cases, because of the limitation in what the Feature protobuf supports. Also there's the int32_t special casing.

162

No, and we may actually yank out Effects - we don't currently use it.

382–384

Maybe, but we need to evaluate that alternative. Effects should be the same as decision, except if somehow inlining failed for a correctness reason we didn't detect upfront. So training, currently, may end up being confused by this aspect.

yundiqian accepted this revision.Aug 7 2020, 1:53 PM
This revision is now accepted and ready to land.Aug 7 2020, 1:53 PM
This revision was landed with ongoing or failed builds.Aug 7 2020, 2:57 PM
This revision was automatically updated to reflect the committed changes.
mtrofin marked 3 inline comments as done.