[llvm][Support] Enable pass timing in StandardInstrumentations constr…

[tarunprabhu]

…uctor

Add two optional arguments to the llvm::StandardInstrumentations constructor that enables pass timings and pass timings per run. If the optional arguments are not provided, the TimePassesIsEnabled and TimePassesPerRun global variables will be read when constructing the TimePassesHandler object which will preserve the current behavior.

---

Notes for reviewers: This was motivated by #107270 which could be simplified if timing of passes could be enabled without having to set llvm::TimePassesIsEnabled and llvm::TimePassesPerRun directly.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[tblah]

This looks okay to me but please wait for approval from somebody who contributes to this area.

One alternative would be to make another constructor with these two arguments (as bools not optionals) and delegate to that from this one. I don't personally feel strongly enough to ask you to change it, but it might fit better with the style used in these classes.

StandardInstrumentation::StandardInstrumentations(LLVMContext &Context, bool DebugLogging, bool VerifyEach, PrintPassOptions PrintPassOpts) :
   StandardInstrumentations(Context, DebugLogging, VerifyEach, PrintPassOpts, TimePassesIsEnabled, TimePassesPerRun) {}
   
StandardInstrumentation::StandardInstrumentations(LLVMContext &Context, bool DebugLogging, bool VerifyEach, PrintPassOptions PrintPassOpts, bool EnableTimePasses, bool EnablePerRunTiming) :
  TimePasses(EnableTimePasses, EnablePerRunTiming),
  OptNone(DeubLogging),
  [...]

[aeubanks]

in general this direction makes sense

I'm not really a fan of the std::optional<bool> though. what about just making the param a bool that defaults to false, and the cl::opt flags override it if they are explicitly specified?

also, we really should make the StandardInstrumentations constructor take a StandardInstrumentationsOptions as opposed to a bunch of separate params, but that can be a followup

[tarunprabhu]

Thanks for taking a look, Arthur.

I'm not really a fan of the std::optional<bool> though. what about just making the param a bool that defaults to false, and the cl::opt flags override it if they are explicitly specified?

If I understand you correctly, you are suggesting

 TimePasses(TimePassesIsEnabled ? TimePassesIsEnabled : EnableTimePasses,
            TimePassesPerRun ? TimePassesPerRun : EnablePerRunTiming)

I think explicitly specifying the option will set TimePassesIsEnabled to true. Of course, that variable is exposed, so it can also be set to true without use of a command-line option which is what clang currently does.

also, we really should make the StandardInstrumentations constructor take a StandardInstrumentationsOptions as opposed to a bunch of separate params, but that can be a followup

That is a good idea. I am happy to take a stab at it later.

[aeubanks]

Thanks for taking a look, Arthur.

I'm not really a fan of the std::optional<bool> though. what about just making the param a bool that defaults to false, and the cl::opt flags override it if they are explicitly specified?

If I understand you correctly, you are suggesting

 TimePasses(TimePassesIsEnabled ? TimePassesIsEnabled : EnableTimePasses,
            TimePassesPerRun ? TimePassesPerRun : EnablePerRunTiming)

I think explicitly specifying the option will set TimePassesIsEnabled to true. Of course, that variable is exposed, so it can also be set to true without use of a command-line option which is what clang currently does.

I meant TimePassesIsEnabled.getNumOccurrences() ? TimePassesIsEnabled : EnableTimePasses. So only use it if it's explicitly set.

And ideally we'd change clang to also use this rather than setting llvm::TimePassesIsEnabled

[tarunprabhu]

Ah, thanks. I didn't know about getNumOccurences(). I'll make that change now.

[tarunprabhu]

I meant TimePassesIsEnabled.getNumOccurrences() ? TimePassesIsEnabled : EnableTimePasses. So only use it if it's explicitly set.

Ugh! So this gets a little messier. The actual command line option is static in llvm/lib/IR/PassTimingInfo.cpp. TimePassesIsEnabled is a global that is the cl::Location for that option.

To do as you suggest, that cl::opt would have to be exposed. If that is ok, I could do it, but it seems to be a step "backwards".

And ideally we'd change clang to also use this rather than setting llvm::TimePassesIsEnabled

I think part of the trouble there is that the legacy pass manager still uses this global. In clang, llvm::TimePassesIsEnabled is set in order to print timing information when -ftime-report is passed on the command-line. We might have to wait until the backends switch to the new pass manager before we can stop setting it in clang. This is the same issue that we are running into with flang as well.

[aeubanks]

I see...

But then if we already need to set llvm::TimePassesIsEnabled to get backend timers, this PR makes the middle end and backend diverge and now there are two things you need to do to enable timers (set llvm::TimePassesIsEnabled and set the StandardInstrumentations constructor) instead of one. Ideally we'd get both the middle end and backend in a similar state to enable timers?

[tarunprabhu]

The idea was to allow enabling timers using either the StandardInstrumentations constructor, or via the command line. In the first version of this PR, the arguments to the constructor were std::optional<bool> that defaulted to std::nullopt. If the arguments were not provided, the command-line would be used to determine if the timers were to be enabled. If arguments were passed to StandardInstrumentations, those would take priority and, potentially, override the command line options. For clang and flang, this is not an issue. The other LLVM tools that use StandardInstrumentations would not see any change in behavior.

However, this would only make things "cleaner" for the new pass manager. We could try to expose just this tiny bit of functionality in the legacy pass manager too, but I haven't looked too closely at that code to see if it would be feasible.

[tarunprabhu]

I think it would be acceptable, for flang at least, to only enable timing of the middle-end passes for now. But I would need to solicit more feedback from that community to be certain. It would be better if we didn't have to resort to setting global variables there. Such concerns have also been raised there for some other cases too.

My intention was to try and set things up so we could slowly migrate to using constructor arguments to enable timings in the pass manager instead of relying solely on globals.

[tarunprabhu]

Closing this since @aeubanks concerns are valid. One ends up having to do two different things since the legacy pass manager used by the backend still requires global variables to be set in order to enable timings. A different approach may be warranted.