Page MenuHomePhabricator

Refactor TimeProfiler write methods (NFC)
ClosedPublic

Authored by amonshiz on Feb 12 2020, 3:46 PM.

Details

Summary

Added a write method for TimeTrace that takes two strings representing
file names. The first is any file name that may have been provided by the
user via time-trace-file flag, and the second is a fallback that should
be configured by the caller. This method makes it cleaner to write the
trace output because there is no longer a need to check file names at the
caller and simplifies future TimeTrace usages.

Diff Detail

Event Timeline

amonshiz created this revision.Feb 12 2020, 3:46 PM
rnk added a comment.Feb 12 2020, 3:54 PM

Once the error handling is fixed up, I'm not sure this is that much better than what came before, but I could be wrong.

llvm/lib/Support/TimeProfiler.cpp
296

Maybe passing the error back to the caller (as the Error type, perhaps?) would be better. LLD would hook it into its diagnostic machinery and clang to its own. Otherwise, the user can supply an unwritable path which will cause clang/lld to print a banner asking them to file a bug report, which is incorrect, it's a usage bug, not a compiler bug.

amonshiz planned changes to this revision.Feb 12 2020, 6:52 PM
amonshiz marked an inline comment as done.

I'm not sure this is that much better than what came before, but I could be wrong.

The benefit of this change is that in the next diff in this stack I add additional usage of the TimeTraceScope to LLVM passes/pass managers. In doing so there is another need to essentially flush the trace info to a file and also validate the file name provided. It made sense to move handling whether or not a user provided a file name to a consistent place.

llvm/lib/Support/TimeProfiler.cpp
296

Will do! Sounds like a good improvement.

amonshiz updated this revision to Diff 245177.Feb 18 2020, 8:56 AM

Add Error based handling

Thanks for the review @rnk

modocache accepted this revision.Feb 19 2020, 5:55 PM

This looks good to me, it sounds like all of @rnk's comments were addressed, and I'd like to unblock D74516 that's stacked on top of this, so I'm going to accept. @amonshiz do you want me to commit this for you? (And @rnk, any comments before I do?)

This revision is now accepted and ready to land.Feb 19 2020, 5:55 PM
amonshiz marked an inline comment as done.Feb 19 2020, 6:18 PM

@modocache that would be great if you could land this for me!

This revision was automatically updated to reflect the committed changes.