Factored out the logging facility, to allow its reuse outside the
inliner.
Details
- Reviewers
ebrevdo yundiqian gjain - Commits
- rG36bb1fb1fe62: [MLInliner] Factor out logging
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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. |
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? |
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. |
add the documentation about calling print() after the trace is finished