- User Since
- Apr 21 2015, 2:43 AM (178 w, 3 d)
See my comment to D52329. Analysis isn't the right place for this implementation. This should be part of the Passes library, and even live directly in the StandardInstrumentations implementation.
Some inline comments.
This looks nice! At this point, I think it makes sense to just move the entire IR Printing implementation here. I don't even think it needs to be mentioned in the header. It doesn't have any other clients, and Analysis is definitely the wrong home.
Wed, Sep 19
I don't fully understand the rationale behind this. How is this different compared to a before/after instrumentation matching the PassManagers?
So I'd still prefer a generic free getAnalysisResult, plus specializations for the non-simple cases, e.g. Loop and CGSCC. That way there's less surprises should there ever be a 'weird' AnalysisManager out there using a different argument mapping. Happy to do this in a followup though!
Tue, Sep 18
Sat, Sep 15
Fri, Sep 14
Note that I'm not saying we should abandon the -print command line options, just that the ShouldPrint functions are the entirely wrong place to access them. This indirection is simply not necessary anymore.
This implementation is modeled unnecessarily close to the legacy behavior. opt should just manually add the printer instrumentation if a print option is given. In particular., this shouldn't use the ShouldPrint* functions. This way there's no need to involve the PassBuilder API, and you also don't pay for the instrumentation if you don't want to print.
This looks entirely straightforward.
At this point I'm bikeshedding: Should getAnalysisResultgo into the Internal header?
This is looking great. Apart from two things that aren't needed anymore, the last point of discussion is getPassInstrumentation. PassInstrumentationAnalysis shouldn't be a special citizen. What about making this a free function instead? In fact, why not make this a generic helper e.g. in the detail namespace to query _any_ analysis result from a template pass?
Thu, Sep 13
This looks good! I dislike the AnyNumber() expecations you highlighted above, but I see that it's unavoidable without either repeating a lot of the test logic again, or without waiting for something like D39678 to happen.
Thu, Sep 6
I like how simple this is now!
Wed, Sep 5
Here's what I think the ownership layering should look like:
Mon, Sep 3
I meant both your arguments towards passing the real IRUnit and an opaque pass identifier.
Okay, that sounds reasonable.
Reconsidering passing the actual IRUnit to the callbacks, I still think that's not a good idea. Downstream users _will_ eventually use this to hook into the pipeline and mess with the IR somehow. So instead this should be opaque to clients.
IMO passing void* as an opaque ID along with the name is fine. Passing the PassConcept by reference would degrade layering, since it lives in IR, not in Passes like the rest of this.
Instead of using a global timer registry, this should get a per-compilation registry from the PassExecutionCounter.
Wed, Aug 29
Back from vacation, and had time for a closer look.
Aug 21 2018
Thanks for moving this forward!
Aug 16 2018
We probably should just disable the unittest on windows, in particular if(NOT LLVM_ENABLE_PLUGINS AND NOT (ARG_PLUGIN_TOOL AND LLVM_EXPORT_SYMBOLS_FOR_PLUGINS)).
Aug 15 2018
Aug 14 2018
This makes complete sense to me, but I'm wondering whether add_llvm_loadable_module is the right macro to use here in the first place. All it does is call add_llvm_library with a MODULE argument, and set up the install.
Aug 9 2018
I had not, but it should work just as well!
Aug 2 2018
On top of my generic concern regarding the unclean implementation of the ShapeInfo itself, I left you a bunch of inline comments, mostly concerning style.
Jul 27 2018
Does this still allow disproving data dependences?
Jul 26 2018
Since the 8.0 branch is around the corner I'd really like feedback from @beanz whether this makes sense or not.
Jul 25 2018
The link is 404, but I assume it boils down to the same thing as either std::any or llvm::Any. It's not hard to reroll the implementation in isl, but then we have three.
As we discussed offline, std::any doesn't really help polly, because it's a c++17 feature. Instead, what about making the generator itself extensible? Allow users to hook into the generation process and modify whatever happens. Then we can just emit llvm::Any privately. Additionally, this can settle the exceptions/no-exceptions debate.
Jul 23 2018
Fixery to appease the regression tests:
- Back out of cool new api in PPCGCodeGeneration.
- Don't delete Any through void*
- Fix a get_user<const>
Address @bollu's comments
Handle nullptr users correctly
Jul 20 2018
Fail harder if the wrong type is given.
Missed a couple of c-api uses
Realized this is still pending. Added some reviewers.
Jul 18 2018
Abandoning in favor D49486, which subscribed the right list :)
Jul 17 2018
Thank, only nits left!
Jul 13 2018
Should I commit this change, or wait until the generator is updated and recreate it using that?
Jul 11 2018
Jul 10 2018
Is the generator in the repo?
I'd prefer putting it into PPCGCodeGeneration.cpp/PPCGCodeGeneration.h.
What's the relationship between this and D48774?
Jul 6 2018
I raised this concern already on Siddharth's version, but if this will succeed that I'll need to repeat it hear: ShapeInfo is pretty spaghetti, this should be fundamentally redesigned.
I don't like that this messes with global state. This destroys local reasoning and makes refactoring unnecessarily harder in the future.
Jul 3 2018
Thanks for fixing this!
Before going into the details and nits:
Jun 29 2018
I don't know when and how mainline isl will provide maintained C++ bindings, but the idea there is that enum isl_schedule_node_type will not be visible from C++. This is essentially a trick to have "inheritance" on node types in C. Practically, isl is going towards individual node types being specific classes that inherit from the generic schedule_node one. They are likely to feature isa<>-style interface. We can declare a local enum and use it to construct matchers though.
Is there a roadmap or RFC for that? I think that's a great idea.
Jun 27 2018
This was how my API worked:
I like the idea! The API needs to be polished a bit I think.
Jun 26 2018
OK, this looks good! You haven't addressed Micheal's testcase yet for the invariant cascade crossing alias set boundaries. For the sake of progress IMO it's okay to leave a TODO and address it in a followup change. @Meinersbur what do you think?
Jun 25 2018
Jun 24 2018
There is still some clutter left, such as irrelevant metadata, a function declaration, and some computations, such as the value stored to memory.
Jun 22 2018
This is starting to look good! Only cosmetic nits left.
Jun 21 2018
Jun 20 2018
Jun 19 2018
RAII management of the isl_ctx
Jun 18 2018
Free the context in the unittest.
Jun 14 2018
Looks like I forgot to add pollydev as a subscriber. Shall I resubmit the patch?
Add a unittest and address comments
Jun 13 2018
Jun 12 2018
Jun 11 2018
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.
Jun 8 2018
Fix weird iterator comprehension.
Jun 7 2018
Add a comment