User Details
- User Since
- Nov 22 2013, 10:24 PM (372 w, 6 d)
Tue, Jan 5
LGTM, but adding @mgorny and @hctim for D81921, which also tried to address the same issue as D44650 (which this replaces), in case I missed something that the former is addressing that this revision doesn't.
Dec 11 2020
LGTM, from an IR legality perspective, but somebody familiar with GlobalISel should take a look from that perspective - adding authors/reviewers of https://reviews.llvm.org/D65167, which added this code. Also, we should have a test.
Sep 30 2020
Sep 3 2020
Well, the module if freed eventually. Is this issue whether the module if freed before the pass manager? If so I think run-twice will (incidentally also do that). I just though run-twice would catch it, because forgetting to clear such things usually causes incorrect behavior the next time around.
Sep 2 2020
LGTM. Could we have a test for this? There is the -run-twice option to the various tools, which is designed to catch issues like this.
Aug 10 2020
Aug 7 2020
Aug 6 2020
I think that would be fine, but in that case, we should probably get rid of x86andnp entirely and just make (and (not x) y) canonical everywhere. The supported patterns for these are much the same, so if we have both canonical forms, we're just signing up for double the work.
Feb 25 2020
Feb 24 2020
Feb 20 2020
Should probably be an optional feature to the verifier pass, like FatalErrors?
Jan 15 2020
Jinx ;) This took quite some tracking down on our side - too bad we didn't wait a couple of weeks. I do still like the iterator interface better, since it checks against exactly this kind of issue in debug mode. It also avoids the second lookup for the erase. I'll rebase this on top of master.
Jan 13 2020
Turns out this doesn't quite yet address the TODO in Inliner.
Reverted that part of this change for a complete solution in a future
revision.
Jan 10 2020
Dec 12 2019
Dec 6 2019
Oct 7 2019
Sep 25 2019
Sep 16 2019
LGTM. @lebedev.ri, do you have further review?
Sep 11 2019
This broke the build for me locally:
In file included from /home/keno/llvm-project/llvm/lib/DebugInfo/GSYM/FunctionInfo.cpp:10:0: /home/keno/llvm-project/llvm/include/llvm/DebugInfo/GSYM/FunctionInfo.h:36:29: error: declaration of ‘llvm::Optional<llvm::gsym::LineTable> llvm::gsym::FunctionInfo::LineTable’ [-fpermissive] llvm::Optional<LineTable> LineTable; ^~~~~~~~~ In file included from /home/keno/llvm-project/llvm/include/llvm/DebugInfo/GSYM/FunctionInfo.h:14:0, from /home/keno/llvm-project/llvm/lib/DebugInfo/GSYM/FunctionInfo.cpp:10: /home/keno/llvm-project/llvm/include/llvm/DebugInfo/GSYM/LineTable.h:118:7: error: changes meaning of ‘LineTable’ from ‘class llvm::gsym::LineTable’ [-fpermissive] class LineTable { ^~~~~~~~~ [720/3364] Building CXX object lib/Analysis/CMakeFiles/LLVMAnalysis.dir/ScalarEvolution.cpp.o
Aug 9 2019
Accidentally only pushed half the changes
Aug 5 2019
Just out of curiosity, how did you discover this bug?
Ok, I think that'd work for my use case. I'll try it out and post a revision to that extent.
Hmm, we do have a list of all the .cpp files in the tablegen tool in ${ARGN}, and I'd be fine depending on the target libraries. Do you know of a way to emulate whatever handling add_executable is doing for .cpp files to get at the included .h files?
Is there a way to just add the whatever the underlying dependencies are that are missing for the native tool? My concern is that I don't want to build target executables (because the toolchain for that is broken).
Aug 3 2019
Updated to add a test and confine the changes to the wasm backend code.
Jul 30 2019
Jul 15 2019
Jul 4 2019
Jul 1 2019
Use set_target_properties to set EXCLUDE_FROM_ALL on the target rather than using
the global setting.
Jun 25 2019
There was a small omission in this revision that I fixed up in https://reviews.llvm.org/rG5bb0dcd96ec1.
Address review comments and add test case.
Remove extraneous output from error message and add test case.
This looks fine to me now. @reames if you have a chance, I'd appreciate confirmation that your concerns are addressed as well.
Jun 23 2019
Jun 22 2019
Jun 7 2019
Jun 4 2019
So, you'd like to make this a frontend flag in order not to expose it to "regular" end users? Or was it because, well, every other flag we have is a frontend flag?
May 8 2019
Looked straightforward enough. Should be fixed by rPLO360238. I don't usually work on polly, so let me know if I misunderstood something.
May 7 2019
May 3 2019
@vtjnash Was the review comment addressed? If so I assume you want me to land this, since you don't have commit?
@sanjoy Could you take another look to make sure I have addressed your comments to your satisfaction before I commit this?
A few minor tweaks.
Rebase and address review comments.
May 1 2019
Apr 26 2019
Shouldn't instead LLVMCFIVerify be declared using add_llvm_library with the proper dependency on the add_llvm_tool line. It seems like the problem might just be mixing the llvm and the cmake variant of these commands.
Actually, looks like the change to setLoopID is still relevant. I'll submit a new revision with just that change.
Looks like this got fixed by @jdoerfert in rL341926 with a near identical patch and test. No rebase required.
Apr 25 2019
Alright, I have rebased this revision. I'd be happy to make the improvement to getAddExpr requested by @tvikram, since that seems independently useful, but I'd like to come to an agreement with @sanjoy and @mkazantsev on direction first.
Undo a small spurious change I noticed while browsing the diff
Rebased
Apr 4 2019
Apr 3 2019
Inline comment on implementation. The functionality looks fine to me.
Jan 23 2019
Sep 12 2018
Fix comment as requested by review, fix a small bug found in more extensive testing,
add a test case for the latter.
Sep 9 2018
Aug 11 2018
Fix ir names in tests broken by previous commits and proplerly match u/smin.
I have a general concern against such changes. In fact, you are introducing an alternative way to express the same thing. umin(a, b) and umax(~a, ~b) are the same, but now have 2 possible notations. It means that whatever pass or analysis that needs to recognize this pattern needs to be aware of both. Whenever the new node was not supported, it might be missed optimization opportunities. And we cannot know for sure how many such places there are, or will be. What motivation do you have for making this change? Is it strong enough to take a risk of missing optimization opportunities that I've just pointed out?
Aug 5 2018
Fix a small bug discovered during production testing (
when expanding umin/umax, don't go to integers just because
types are unequal - the only reason to do is if one of the
operands in an integer and the other is not).
Aug 1 2018
Jul 30 2018
Jul 25 2018
Add a small fix found during testing that I forgot to commit.
May 22 2018
If my memory recovey is successful and I understand the problem correctly, the best solution to me is to separate the mutation and analysis code in StackProtector. Suppose we split it into two passes "StackProtectorAnalysis" and "InsertStackProtectorPrologue", will it solve the problem? During the re-run, we only run StackProtectorAnalysis, but never again insert prologue. StackProtectorAnalysis should be kept as-is, without more special logic to detect whether the prologue is already generated (as this patch does). What do you think?
May 17 2018
This was merged in rL322311, closing to get it off my dashboard. In the future, ideally add
Differential Revision: https://reviews.llvm.org/D41960
with that syntax to the commit message, so Phabricator can close the revision automatically.
May 16 2018
Remove incorrect comment from test case.