User Details
- User Since
- Aug 14 2020, 11:14 AM (104 w, 2 d)
Thu, Aug 11
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.
Mon, Aug 8
LGTM. Thanks.
Fri, Aug 5
Sorry for the delay, missed this before going on vacation.
Wed, Jul 27
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...
Tue, Jul 26
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!
Dec 16 2021
Failing build: https://lab.llvm.org/buildbot/#/builders/42/builds/2800
Dec 15 2021
It was suggested by @reames that -exec-on-ir-change=exe is a better name for the option. This sounds like a good suggestion to me so I have renamed it.
renamed option to -exec-on-ir-change
Dec 14 2021
Dec 13 2021
I see. Yes, that would be a nice feature.
Yes, it would be nice. BTW, the choice is not hard coded but is specified with DefaultDOTGraphTraits::renderNodesUsingHTML() and stored in the ctor since it is unlikely that an HTML and non-HTML mix would be desirable.
Yes, bugpoint will find a misbehaving class but that is just one example usage for this. test-changed is more general in that any script/exe can be called each time the IR changes. Consider the situation where a performance degradation is noticed so the code still compiles but is not as well optimized. bugpoint (as I understand from the documation) will not detect this because the IR still compiles and the exe still runs. Using test-changed and a script that calls llc followed by a timed execution, one could determine how much impact the various passes have on the performance of the code as it gets changed in the pipeline. This could point out the source(s) of speed-ups and slow-downs in the pipeline and could be used as an exploration tool.
Dec 9 2021
Replace the shell script with just calling cat directly. cat is used in many lit tests and will cat the IR file and will give an error that no file exists with the name of the pass and this can be checked. @thakis, can you please verify that this does not break the windows build?
@aeubanks Thanks for reverting it. I'll take a look.
Dec 8 2021
Dec 7 2021
I've updated the patch with some changes to same parameter names (V -> Value, etc) and corresponding comments. The only substantive change in the update is that I changed DotCfgDiffNode::getEdge to DotCfgDiffNode::getEdgeColour and had it return a StringRef to the colour instead of the std::pair it previously did since the only use just extracted the colour from the pair (lines 1496 and 1769).
Dec 6 2021
Dec 2 2021
ping
The dot documentation I saw recommended this method of checking whether an output format was supported.
Nov 30 2021
ping
Nov 8 2021
Respond to review comments: remove templates in functions for temporary file creation and cleanup.
Nov 4 2021
Hi @uabelho, the function is unnecessary and has been removed in https://reviews.llvm.org/D113188
Hi @hvdijk, a fix for the lit-test problem is up for review in https://reviews.llvm.org/D113187
Nov 2 2021
Hi @tpopp, yes, it looks fine. Thanks!
Hi @tpopp, can you please explain to me the nature of these failures? The code changed here is only active under options that are not being used in those tests so I expect that the errors are unrelated. I have not received any notifications from the build-bots of a problem so far. I am also unclear as to why there are build issues on those other platforms. Are you asking me to revert this?
Nov 1 2021
Respond to review comments, remove nolint instructions and clean
up unit tests.
Oct 19 2021
Update after landing of D110737 which had most of the changes to existing code.
Oct 7 2021
missed @MaskRay 's comment, will add that before attempting a relanding
LGTM, thanks
As I recall, I added this in based on comments in the review and I am fine with removing it. However, the parameter to unWrapAndPrint was added to preserve existing function and if this is removed, then the parameter should also be removed as it will always be false. The parameter can be dropped from the call to print also, as it defaults to false.
Oct 5 2021
Remove unnecessary include.
Oct 4 2021
Added the usual filtering options (filter-passes and filter-print-func) with uninteresting passes being saved with a message that they were filtered out. This should speed up things significantly when they are used. If the user isn't aware of the name of the crashing pass, they can use -filter-passes=blah (where blah doesn't exist) and they will get a message saying that the pass was filtered out. They can discover the name from that message and then rerun it with -filter-passes.
Sep 30 2021
I think I am getting confused with llvm-extract --func=<function> which will extract what is needed for a function from a module IR. However, one could likely use that to get an IR with the necessary addition declarations and then substitute in the IR for the function using this option. I wonder if it might be possible to use the code in llvm-extract to do that automatically? Something to think about...
Regarding always printing the module, I'm not sure that is desirable. The
problematic IR may be very large and the option respects -print-module-scope
so the module IR can be obtained. There are also tools for creating a fleshed-out IR
from the IR for a function.
Sep 29 2021
As requested, I removed the mutex code and cleaned up a few things.
I agree that the mutex code is overkill, considering that this is intended for debugging. If things go off the rails because of the lacking mutex code, the consequences are not severe since it is expected to crash anyway (or else why would you use this?)
I will removed the mutex code and clean things up based on your comments unless someone raises an objection.
Created new review https://reviews.llvm.org/D110737 with non-functional background changes to simplify this review. I will update this review to reflect any changes from that review when it lands.
Sep 28 2021
fix Clang-tidy problems
Thanks. Would it help if I split it up a bit? For example, there are a lot of changes relating to making data into templates which affects the existing change printers but doesn't change their functioning. These could be split out and handled first to simplify things. Would this help?
Update and change test pass to use assert(false)
I do not know what is causing the debian build to fail but I do not believe the failure is caused by my changes. I used the instructions to replicate the failure and this resulted in the same error that llgo isn't a known project. Checking out the previous commit resulted in the same error so it is a pre-existing condition and not the result of my changes.
Sep 20 2021
make clang-tidy comments specific to readability-identifier-naming
Add // NOLINTNEXTLINE comments
Sep 16 2021
The clang-tidy complaints relate to names that are required by existing code.
Sep 15 2021
Fix clang tidy problems and add option to specify dot executable.
Fix lit test failure: make test unsupported if /usr/bin/dot is unavailable.
Removed stale parent reviews because I think they were causing problems with applying the patch
Trying again with a newly generated patch...
Trying newly generated patch as last one couldn't be applied. I also added code to call expand_tilde on the -dot-cfg-dir specified directory and then make it absolute.
Sep 14 2021
I have addressed your comments from previous reviews. Regarding the statics to avoid a crash,
I figured out that the problem was a template instantiation that was missing a const, resulting
in a copy being made, which didn't copy everything. This has been fixed. I have also
simplified the code and templates.