This is an archive of the discontinued LLVM Phabricator instance.

ManagedStatic: eliminate uses for cl::opt in the llvm directory
Needs ReviewPublic

Authored by nhaehnle on Jul 5 2022, 2:14 AM.

Details

Reviewers
efriedma
lattner
Summary

Bring those uses more in line with the pattern used in MLIR, i.e.
pack the cl::opts into a struct. We avoid ManagedStatic entirely by then
instantiating that struct as a static function variable.

Remove additional uses of ManagedStatic in the affected files at the
same time.

Diff Detail

Event Timeline

nhaehnle created this revision.Jul 5 2022, 2:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2022, 2:14 AM
nhaehnle requested review of this revision.Jul 5 2022, 2:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2022, 2:14 AM
efriedma added inline comments.Jul 29 2022, 2:19 PM
llvm/lib/Support/Timer.cpp
47

Is the problem described by this comment not an issue anymore? I guess the timing of the call to initTimerOptions is more predictable since we aren't using a global variable, but is it guaranteed to happen before the StatisticInfo constructor runs?

nhaehnle added inline comments.Aug 2 2022, 1:25 PM
llvm/lib/Support/Timer.cpp
47

I don't think so, and haven't seen any test issues.

The theoretical justification is: previously, the string was owned by LibSupportInfoOutputFilename, which is seeded from initTimerOptions via getOptions.

With this change, it is owned directly by Options::InfoOutputFilename, which is also seeded from initTimerOptions via getOptions.

Previously, there were multiple globals that had to be coordinated somehow. With the change, everything is owned by a single global.

Now that said, revisiting this I just realized that there is a real change here, which is that if CreateInfoOutputFile is called *without* first calling initTimerOptions, the change will now implicitly create the cl::opts, which was not the case previously.

Is that a problem? Calling CreateInfoOutputFile without first initializing this module seems like a weird thing to do.

efriedma added inline comments.Aug 2 2022, 1:46 PM
llvm/lib/Support/Timer.cpp
47

The issue would be if the StatisticInfo destructor runs after the string is destroyed. To avoid this, we need to ensure the "Options" struct is constructed before the StatisticInfo constructor returns.

Now that said, revisiting this I just realized that there is a real change here, which is that if CreateInfoOutputFile is called *without* first calling initTimerOptions, the change will now implicitly create the cl::opts, which was not the case previously.

Is that a problem? Calling CreateInfoOutputFile without first initializing this module seems like a weird thing to do.

This doesn't seem likely to cause a problem.

nhaehnle added inline comments.Aug 2 2022, 2:13 PM
llvm/lib/Support/Timer.cpp
47

Ah, I think I see your point now, but now I'm at the point where I'm doubting whether the unchanged code is really robust.

First off, the comment is wrong: ManagedStatics do get destroyed, when llvm_shutdown is called. So the status quo relies on getLibSupportInfoOutputFilename being called before the StatisticInfo is constructed (or, possibly, being called only when the StatisticInfo is destroyed).

The StatisticInfo constructor does call ConstructTimerLists to address a similar dependency, but that doesn't seem to trigger a call to getLibSupportInfoOutputFilename.

It seems to me that (orthogonal to this change) the StatisticInfo constructor should (indirectly) call getLibSupportInfoOutputFilename. Does that make sense to you? I can draw up a patch for that tomorrow.

efriedma added inline comments.Aug 2 2022, 2:36 PM
llvm/lib/Support/Timer.cpp
47

Seems fine.