- User Since
- Nov 22 2013, 10:24 PM (294 w, 3 d)
Thu, Jul 4
Mon, Jul 1
Use set_target_properties to set EXCLUDE_FROM_ALL on the target rather than using
the global setting.
Tue, Jun 25
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.
Sun, Jun 23
Sat, Jun 22
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.
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
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.
Add LLVM_DUMP_METHOD to the added dump() method.
May 12 2018
Updated to use getLoopLatches and add a test.
Yeah, I've been thinking about what the best way to test this is. It seems somewhat hard to reliably trigger
a verifier fault by removing an instruction - llvm.stackprotector is the one I see most commonly, but I don't
really want to make the test depend on that behavior. One option is to teach bugpoint to use a custom verifier
pass and then write a custom verifier pass that e.g. checks for the presence of a call. That seems like a decently
attractive option, because it would be a useful feature in its own right. Some frontends have their own invariants
and corresponding verifier passes that they want to maintain in their early passes. Would be nice if bugpoint could
support that. The only thing I'm not quite sure off is to how to do this nicely. Presumably, we could use the analysis
name as a pass and run that through the pass manager. I'm just not sure how to get the analysis result out of that.
May 11 2018
May 4 2018
Jan 31 2018
This happens fairly frequently for us in julia, because we use i8 to represent tags for tagged unions and then switch on them to discover which of the union components is active.
Without this patch I saw ~3x performance regressions in the worst benchmark (because it prevented other optimizations from seeing optimization opportunities based on the
active union component). I did not see compilation performance impact in my testing.
Jan 18 2018
Jan 11 2018
Looks good to me with the minor tweak to the test. To keep the breadcrumb for future reference, I had changed this in D33179.
Oct 26 2017
Oct 25 2017
error: cannot initialize return object of type 'void *' with an rvalue of type 'FILE *const *' (aka '_IO_FILE *const *') EXPLICIT_SYMBOL(stderr); ^~~~~~~~~~~~~~~~~~~~~~~
Sep 25 2017
Sep 19 2017
this is the julia pass pipeline (https://github.com/JuliaLang/julia/blob/master/src/jitlayers.cpp#L148). IIRC the original list of passes came from VMKit,
but the pass list was adjusted as needed over the years.
Sep 17 2017
Fix a small bug and also look through a single level of bitcasts.
Since IRBuilder automatically inserts bitcasts to i8*, it seems
prudent to handle that case.
Sep 15 2017
Jul 7 2017
The implementation of this looks good to me. I guess I'm not entirely sure why it's necessary for that type to be distinct in the first place, but that's probably a different discussion from just fixing this bug.
Jun 29 2017
Jun 28 2017
Factor out intersection code into a method of AAMDNodes
Jun 20 2017
Jun 13 2017
Jun 10 2017
Having pondered this some more, I wonder if what we're missing is an annotation on the addrspace cast itself that indicates whether or not GEPs may be commuted past it (could call it inbounds or something else). It seems like in many cases (including allocas). The frontend (or whoever else is doing language/target specific work) can often know whether the entire object is available in the target address space (e.g. because the entire stack always is).
LGTM, FWIW. I encountered this issue as well and this seems like the correct solution.
Jun 9 2017
Bump on review for this.
I'm gonna go ahead and commit this anyway, since this fixes a miscompile. If there are any comments I'll take them in post-commit review.
Jun 2 2017
Bump. @sanjoy does this look good to you?
Bump. Would be great if somebody could review this. I know Sink it's not really actively developed, but I think this is a pretty straightforward fix.
Jun 1 2017
Sure, see if you think this is the right way to do it. If so, mark it as accepted, and I'll go ahead and commit it. I feel like we've given other people enough time to weigh in ;).
I don't think that change is entirely necessary. I don't have any strong objections to it, but I also don't see a good reason to require it. In any case, let me get this in to be able to re-land D33655 and we can revisit the more disruptive change later.
There's already such a test case, but the cloning currently doesn't assert properly (but it can generate incorrect code). D33655 fixes that up, so I think the testing is covered once that LLVM commit goes in. I'll hold off a little while to give @aprantl a chance to look at this as well, but I do want to get this in in short order, so I can recommit D33655.
Finalize all subprograms when we're done emitting them.
The one we're interested in is a side effect, but doing
this uniformly might be cleaner and help avoid similar errors in the future.
Sure, I'll commit it.