LGTM

I agree that the potential gain is limited and probably not worth the potential increase in code size. This patch captures the changes/discussion if someone else is interested in the future. I am abandoning this revision.

@nikic, thank you for your comments. I too have concerns about this proposal. I agree that the benefits are marginal but I would like to address some of your comments.

This will show some of the differences:

I was surprised that your change would require test changes--and that probably points out a bug in my initial design. Only the TextChangeReporter unwraps the module to always use the module in handleInitialIR. This should be lifted out to the call-site so it is consistant with all handlers. It doesn't make sense that the initial IR changes based on filtering (ie, which one prints first). But that is not germaine to this change so...

LGTM

I'm fine with handling the return for common as a separate change, if necessary.
Is the error produced now because it passes back the incorrect option rather than quietly changing it to something appropriate as it did before?

ErrorOr isn't really appropriate here because the string was produced when the error occurred and it was None otherwise, which is the opposite of what happens with ErrorOr. It isn't desirable to create the array of filenames using ErrorOr since you want to reuse the existing ones or a large trace will open too many temporary files. However, your comment does point out that this return is inconsistent and awkward. I've changed these functions to return an std::error_code, which is more consistent. It loses the different error strings previously produced, but this code is only used for debugging anyway. Also, I changed the function for creating the temporary files to clean up any it succeeded in creating if an error occurs.

ping

This is out of date and would require reworking.

@aeubanks When I first reverted this because it failed on windows, I wasn't sure what to do to fix it and it fell off my radar.
I have updated it to reflect changes since then (the tests were updated to reflect changes to specifying filtering, functions have moved, etc). I've made a few minor changes, such as the functions for managing the temp files return Optional strings now and the tempfile name is different to reflect the multiple usage. I've also added a lit.local.cfg file to ensure that '/bin/cat' exists, which should fix the original windows failure that caused the revert. Can you please take a look to see that everything is still okay? Thanks.

LGTM

Respond to review comments: remove unnecessary asserts

Is the mode change on check-time-trace-sections.py intended? I did a count of python files under llvm-project and < 10% are 755. I'm not saying it is better one way or the other; I'm just ensuring it is intentional.

@mkazantsev, I have addressed your concerns.
Regarding your comments concerning the use of infinity (as an unsigned) vs Optional<unsigned>: perhaps it's just me, but I found the use of Optional<unsigned> confusing and it became more confusing when I incorporated the max iterations into the algorithm. It isn't clear from the code at this point, but when one expands the algorithm to include binary operators, one wants to avoid computing the number of peels unnecessarily when the first operand has already reached (or surpassed) the maximum allowable peels. This is why I simplified the code to use an unsigned and placed it all within protected members of the class, wrapping it with the Optional<unsigned> on coming out, to both clarify the algorithm (especially when considering max iterations) and also to prevent people monkeying with the unsigned value (as you pointed out as a possibility in your comments). Your criticism of the constant name Infinity is valid--it is misleading. In response, I have hidden the Optional<unsigned> behind a type called PeelCount, used a constant Unknown (which is None) rather than Infinity and kept the math in protected functions. I think this makes the code easier to understand and also addresses your preference for using Optional<unsigned>.

I have split out the refactoring of the code so there are no intended
functional changes. I have added the test case as requested, showing
the current, limited functionality. After this lands, I will post a
patch with the expanded functionality.

ping

I have verified that adding inalloca and removing tail call on stackrestore in my sample IR will fix the problem.

Respond to review comments. Remove templates and introduce
generic range-based singleton search routines.

Yes, I agree it is incomplete (aside from being incorrect here :-) I've just been asking to ensure that my understanding of freeze is correct. Thanks.

In D136284#3874614, @nlopes wrote:

In D136284#3874596, @jamieschmeiser wrote:

In D136284#3874492, @nikic wrote:

At least in C++, working with uninitialized memory is pretty much always immediate undefined behavior, see https://eel.is/c++draft/basic.indet for the relevant wording. The only exception are "copy-like" operations on unsigned character types, which comparisons do not fall under.

I believe the C specification is less clear cut about this, but Clang and LLVM assume basically the same to also hold for C code.

What version of the C++ standard is this? Every version that I have seen has basics as section 3 and I cannot find this section, nor anything similar. Section 6 is Statements.

That discussion is orthogonal to this patch.
This patch is not desired because it's not needed per the current LLVM IR semantics. If you want to change something, you need to start by proposing a change to the LLVM IR semantics. You'll need to justify why it's needed, why it's correct, the perf impact, how to make it backwards compatible, why it's better than the proposals over the table right now.

Anyway, a patch like this solves no problem. LLVM allows loads to be duplicated. Your patch does nothing to prevent that and to ensure all loads see the same value. The issue is way more complicated than what this patch implies.

I checked with a member of the C++ standards committee and he verified that comparing an uninitialized value against itself is, indeed, undefined behaviour, in the general case. I am abandoning this revision.

In D136284#3874492, @nikic wrote:

At least in C++, working with uninitialized memory is pretty much always immediate undefined behavior, see https://eel.is/c++draft/basic.indet for the relevant wording. The only exception are "copy-like" operations on unsigned character types, which comparisons do not fall under.

I believe the C specification is less clear cut about this, but Clang and LLVM assume basically the same to also hold for C code.

In D136284#3869216, @nikic wrote:

The current behavior here is intentional -- in fact, LLVM will move towards returning poison for loads from uninitialized memory in the future (though precisely how this will happen is still uncertain, see https://discourse.llvm.org/t/rfc-making-bit-field-codegen-poison-compatible/63250 for some recent discussion on the topic).

In D136284#3869306, @nlopes wrote:

However, multiple loads of the same memory must be equal

This premise is incorrect w.r.t. with current semantics, which say that loads return undef for uninit memory.

As Nikita mentioned we are working towards changing this semantics. The work is ongoing, but we pivoted to make a couple of improvements in other areas to avoid perf regressions. For example, see the clang !noundef patches.

I found the following in clang/lib/CodeGen/CGCall.cpp:

// Insert a stack save if we're going to need any inalloca args.                                                                                                                                                                                            
if (hasInAllocaArgs(CGM, ExplicitCC, ArgTypes)) {
  assert(getTarget().getTriple().getArch() == llvm::Triple::x86 &&
         "inalloca only supported on x86");
  Args.allocateArgumentMemory(*this);
}

What does inalloca mean? It is only mentioned in the syntax line of https://llvm.org/docs/LangRef.html#alloca-instruction.

I think it will fail even if stackrestore is not tail call because it will still not pass the !isStaticAlloca query

ping

Great. Also, something you may want to consider, either as part of or after you land this code. This really is a specific instance of a more generic problem: setting up option handling for something that saves results in a file for each compilation. This is equally useful for other things such as listings and could also be used by something like print-changed (which currently just outputs to the stream), opt stats reporting, etc. This code could be organized as a function (or possibly an object, depends...) that takes a string for the extension, a lambda/template for the virtual call on whether to add the option to a tool so that off-handling, platform-isms, and where files are saved would all be captured neatly and would be re-usable. InferTimeTrace and getPath, off-loading, platform-isms would be captured in a generic call that would look something like (in this instance)

PerFileTraceGenerator(".json",
     [](Tool &T, Args &Args)->bool{ return T->supportsTimeTrace() && Args.hasArg(options::OPT_ftime_trace, options::OPT_ftime_trace_EQ; },
     "-ftime-trace=");

Each option that needs per file output would just call this function appropriately.

I had an email exchange with @dongjunduo about this situation. He was a GSoC student that @Whitney and I were mentoring for the past summer. He agrees that your approach is cleaner. There appears to be two parts to your work. First, you implemented the determining and passing of the options differently, and secondly, you improved the handling of off-loading and system specific file handling. Based on your earlier response, we proposed to him the following and he agrees that it seems appropriate. Could you please add comments to https://reviews.llvm.org/D131469 and he will work with you to change his code to reflect searching for -o and using the virtual functions. Then, if @MaskRay agrees, he can land his code and finish up his GSoC work. You can then add your extensions of off-loading and file-handling. Is this acceptable to you?

I'm a little confused as to what is being proposed here. Is this building on D131469 or is it an alternative? It seems that there are portions of the code from D131469 included in these changes, which implies that you are building on it. I think this patch is premature in that the other patch has not yet landed (@MaskRay has asked for revisions that @dongjunduo has made but is waiting for review). When that patch has landed, this could be reposted based on those changes.

I agree. We could time the pass, figure out whether it changed anything (outside of the timing) and then decide how to save the timing info for the pass.

@aeubanks An interesting idea. I'll give it some thought... it should be doable by combining this with the logic in the change printers but I'd be concerned that the detection of the changes would become a hotspot itself. Perhaps a double run, the first to determine which passes make changes and then a recompile to time those passes that are then known to make changes.

or do you mean noting which passes don't make changes and calculating the time spent in those passes? That would combine this work with the print-changed type filters...

@aeubanks Not sure what you mean... something like print-changed=no-change? That would different from this work but we can continue the discussion.

I have delivered https://reviews.llvm.org/D133587 so you should be good to go without the FIXME or changes.

As discussed, change your code to use the name of the loop or call getIRName and add a FIXME comment to resolve the 2 pieces of code. https://reviews.llvm.org/D133587 has been created to fix the generation of the loop names and fix the lit tests.

A couple of nits left: small edits to comments and the change of "...() == nullptr" to "!...()" and it is good to go.
LGTM.

If the source is specified with a partial path, I would expect the .json file to be in the same directory as the source, so dir1/f.C and dir2/f.C would result in dir1/f.json and dir2/f.json. But this can be a later improvement. Let's get this landed.

You want the timing extensions to be the last ones called before the pass and the first ones at the end of the pass so that the overhead of the other callbacks is not included in the timings. Does PassInstrumentationCallbacks have the ability to specify that the pass be registered as the first callback? If not, add a bool parameter that allows it to be placed at the front with a default of false, and then put all timing extensions at the end, with the parameter being set to true for the registering of the atEnd callback (to put them as the first ones called). Also, add a comment that the timing ones should be last for this reason.

Considering that getIRName is only used for identifying the IR, do you even need the change?

Good progress.

You should not have debugging information in code that is up for review. If this is debugging information that you plan to leave in for future purposes (which I doubt is the case here), you need to protect it so that it isn't active unless some option is set. For example, see LLVM_DEBUG code that lives in various opt routines.

LGTM. Thanks.

Sorry for the delay, missed this before going on vacation.

I'm wondering what would happen (after this change) if a function were to re-order the blocks (without any other change). EG, the first 2 blocks in the function are swapped in the function list of blocks with no other change. I suspect that this would then indicate that the change was the original block 1 was replaced with the new block 2 and the original block 2 was replaced with the new block 1 with the corresponding changes to the branches. I think that may be unavoidable since the comparisons are string based. Besides, it is better than crashing...

Since you have the before and after versions, I expect you could get -print-changed=diff and cdiff without too much difficulty by calling the diff routine that the opt print-changed options use, if you want to expand this functionality.

LGTM. Thanks.

In D128048#3607295, @MaskRay wrote:

You may add -ftime-trace=<file> instead of introducing a new spelling.

In D128048#3601588, @Whitney wrote:

In D128048#3601579, @jamieschmeiser wrote:

Can you please use git rebase -i to collapse all the changes into a single change? If this isn't done, it is difficult to know what is being reviewed as the changes only show the differences since your last revision, not all of the changes.

I can see all the changes, make sure your link is not suffix with "new", or check the history tab to see what is it comparing.

Can you please use git rebase -i to collapse all the changes into a single change? If this isn't done, it is difficult to know what is being reviewed as the changes only show the differences since your last revision, not all of the changes.

This is a good start. You should mention in the summary that when -c is not specified, the compiler is invoked with -o pointing at /tmp so the .json file will currently be placed there, which is confusing.

Respond to review comments: add cpp test.

Thanks!

Failing build: https://lab.llvm.org/buildbot/#/builders/42/builds/2800