- User Since
- Apr 21 2015, 2:43 AM (164 w, 6 d)
Free the context in the unittest.
Thu, Jun 14
Looks like I forgot to add pollydev as a subscriber. Shall I resubmit the patch?
Add a unittest and address comments
Wed, Jun 13
Tue, Jun 12
Mon, Jun 11
As I mentioned in person: Instead of detecting this specific pattern, would it be possible to treat this as a fixpoint iteration? I.e., maintain a set of undecided address sources and keep shrinking it until only the non-invariant ones remein.
Fri, Jun 8
Fix weird iterator comprehension.
Thu, Jun 7
Add a comment
Wed, Jun 6
This does not look correct. Consider the AA.ll testcase. Your change makes DA claim there is no output dependency on the load in %for.inner, but there is! Carried over the outer loop. So the result should be input[* =] (don't quote me on the syntax though).
Interesting, I was under the impression that the relevant differences between MODULE and SHARED were only on Mac. In either case, this change LGTM.
Mon, Jun 4
What platform does this occur on? Mac?
I love the gist of this, but there are a couple of implementation issues, some of which @Meinersbur pointed out already.
Some inline nits. What worries me slightly is that the test oracle changed in GCD.ll. Why is that? Where the tests plainly wrong before?
May 18 2018
Thanks for the clarification @rnk!
I agree with @rnk, too, the question is do we want to enable this in the meantime?
There is this ancient commit here D18826 that introduced auto-export of all symbols. Shouldn't this work here as well?
I'm likely not the right person to review this, I know next to nothing about windows dev.
May 16 2018
Since this is only for the testcase, sounds perfect!
Fundamentally looks good, but I'm slightly concerned about using different macros in the Code (LTDL_SHLIB_EXT) vs. in the build (LLVM_PLUGIN_EXT). Right now those are identical, but it might change. Can we export LLVM_PLUGIN_EXT in config.h?
Do you need help committing this?
May 15 2018
Since the diff being broken is mostly a workflow issue, this patch LGTM.
After some digging, I believe this is caused by svn diff comparing the new file against the old one, which is of course unsuitable to create patches from. So you likely should create the diff with svn diff --show-copies-as-adds or svn diff --patch-compatible.
I'm not sure what caused it, but the patch is not applicable.
Actually remove unittest driver as well.
Something is off with the diff. E.g. the file TestPlugin.cpp doesn't exist, yet the diff contains modifications. Did you miss a local commit?
May 8 2018
May 2 2018
I'm generally fine with this, just one more nit: I don't like for the argument to be called Aborted, for that suggests an error condition. What about BreakIteration instead?
This is a use-after-free.
Apr 29 2018
Do you plan on actually commiting this to the repository, or would you like any feedback nontheless?
I think this diff is too large and should be split into at least two parts:
- The implementation of isl::quota, and possibly a small unittest showcasing how it's intended to be used
- Update of the APIs
Apr 28 2018
I think this makes sense, with some inline nits.
Apr 27 2018
@RKSimon Is this still happening?
Apr 25 2018
LGTM. Thanks for fixing!
Apr 20 2018
Apr 19 2018
LGTM, and solves the issue locally.
Apr 17 2018
Apr 16 2018
In a legacy PM pipeline, basic-aa is always the default. I.e., if you do AA queries without specifying any AA passes, you always get basic-aa results. In the new PM, that's not true. So i think it should be present in the testcases. I'm happy to add it to `%loadPolly', though!
Add a testcase.
Apr 11 2018
Since you expect users to include the header, the operators should also be declared static.
I can add it to %loadPolly. I'm hesitant to make this opt-in, though, since I fear the tests could get very flaky.
Apr 10 2018
The tests for this I added in a follow-up revision D45493, which, as you can see, is quite large. I wanted to keep the functional change here and the mechanical changes to the tests separate for better review.
It's obviously hard for reviewers to examine all changes, but I'd be happy if you could take a look at a random sample :)
Apr 5 2018
This patch only changes the layering for ScopPasses. Note that Scop analyses are still being proxied in and out of function. As a consequence, function layer passes are not affected by this change.
Apr 4 2018
Address review comments:
- Use more StringRef and Twine
- Hide the PassPluginLibraryInfo object
Apr 3 2018
Mar 28 2018
Rename, move around, and simplify some things.
Mar 26 2018
Mar 14 2018
Address review comments, mostly adding commentary and renaming things.
Mar 13 2018
Mar 4 2018
Mar 3 2018
Mar 2 2018
Improve Layering: Move New-PM Pass Plugins into the Passes library
Feb 28 2018
My bad, I uploaded the reverse diff ...
It was in the PR, but I'll mention it in the commit.
This is a fix for existing currently failing testcases, I think that should cover it.
Jan 11 2018
Dec 6 2017
Remove isl ownership markers
Dec 4 2017
Dec 2 2017
Dec 1 2017
Actually the tests requires some changes.
Nov 30 2017
I think this should be an RFC on llvm-dev. This needs a wider audience and a discussion that goes beyond the technical.
Nov 28 2017
I think you have to recommit this, since phabricator doesn't send out the original patch if you forget to add llvm-commits up front :(
Nov 27 2017
@bollu: Can you verify that this fixes your issue?
Nov 20 2017
If this is going back into LLVM, it should also be a NewPM Pass.
Nov 19 2017
Nov 17 2017
Address review comments.
Nov 16 2017
Okay. In that case LGTM.
Shouldn't this actually go to errs() instead?