User Details
- User Since
- Aug 14 2020, 11:14 AM (163 w, 2 d)
May 25 2023
LGTM
May 23 2023
Jan 31 2023
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.
Jan 27 2023
@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.
Jan 24 2023
Jan 20 2023
Jan 11 2023
This will show some of the differences:
Jan 6 2023
Dec 13 2022
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...
Dec 12 2022
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?
Dec 5 2022
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.
Dec 2 2022
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.
Nov 25 2022
Nov 23 2022
@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>.
Nov 22 2022
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.
Nov 21 2022
Nov 17 2022
ping
Nov 4 2022
Oct 28 2022
Oct 27 2022
I have verified that adding inalloca and removing tail call on stackrestore in my sample IR will fix the problem.
Oct 26 2022
Oct 25 2022
Respond to review comments. Remove templates and introduce
generic range-based singleton search routines.
Oct 21 2022
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.
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.
Oct 19 2022
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
Sep 19 2022
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.
Sep 18 2022
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?
Sep 13 2022
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.
Sep 9 2022
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.
Sep 7 2022
A couple of nits left: small edits to comments and the change of "...() == nullptr" to "!...()" and it is good to go.
LGTM.
Sep 6 2022
Sep 2 2022
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.
Sep 1 2022
Aug 23 2022
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.
Aug 22 2022
Aug 18 2022
Considering that getIRName is only used for identifying the IR, do you even need the change?
Aug 17 2022
Good progress.
Aug 11 2022
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.
Aug 8 2022
LGTM. Thanks.
Aug 5 2022
Sorry for the delay, missed this before going on vacation.
Jul 27 2022
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...
Jul 26 2022
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.
Jul 11 2022
LGTM. Thanks.
Jun 24 2022
Jun 23 2022
Jun 22 2022
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.
Jun 17 2022
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.
Jun 6 2022
Jun 3 2022
Jun 2 2022
Respond to review comments: add cpp test.
Jun 1 2022
Mar 8 2022
Thanks!