- User Since
- Apr 21 2015, 2:43 AM (187 w, 2 d)
Is the goal here anything beyond makeing -print-after/before work in the newpm?
Fri, Nov 16
Sorry for the delayed response. So the main problem is that AddressSantizer needs to perform some initialization involving reading stuff from global metadata and getting some target specific information. More specifically, just these 6 things which are all initialized in doInitialization:GlobalsMD.init(M); C = &(M.getContext()); LongSize = M.getDataLayout().getPointerSizeInBits(); IntptrTy = Type::getIntNTy(*C, LongSize); TargetTriple = Triple(M.getTargetTriple()); Mapping = getShadowMapping(TargetTriple, LongSize, CompileKernel);
If asan doesn't modify the module for this ('this' meaning the Mapping and the GlobalsMD here), than this is an excellent candidate for an analysis.
Tue, Nov 13
Sun, Nov 11
Sat, Nov 10
Hmm... can you point me where function definitions are being added in doInitialization, I just dont see it (frankly I'm not familiar with this code at all).
I might have been wrong here, I assumed the Asan function pass did this,too, because all sanitizers I've looked at do it. But it's possible that only the Module variant does it.
The problem is not speed but correctness. The initialization adds function definitions, which function passes can't do. So we need to initialize out-of-band, either with a separate module pass, or some function outside of the pass pipeline. I strongly prefer the latter, but it's not entirely clear to me how this function should be called. It's obviousl when running this through clang, but what about opt?
A key difference between this and legacy is at O0, this EP won't be triggered.
Making this a module pass will cost us all the nice chaining and cache locality we get from a function pass. That will make the already slow instrumentation even more expensive. Do you see a way to keep this as a function pass?
Tue, Nov 6
Fri, Nov 2
What is the advantage of this over using command line args?
Tue, Oct 30
It looks like running clang will be good enough. I'm closing this for now!
Tue, Oct 23
Oct 22 2018
The fix looks alright, but did you test it with CMAKE_BUILD_TYPE set though? It's hard to parse with the naked eye, but it looks like you're missing at least a $ there.
Oct 17 2018
LGTM, thank you!
Oct 15 2018
This looks good to me, minor nits inline.
So is this just a bugfix? Or does it add functionality?
What was the deficiency of the initial implementation this is fixing?
Oct 11 2018
You're right, my bad; I missed the fact that it's EP_OptimizerLast, which has no equivalent in the new PM. Your implementation is actually correct here.
This looks great now, thank you!
Oct 10 2018
We're getting there! Some more tiny nits.
Oct 9 2018
I also noticed you got the header in the wrong place. It should live in Transforms/Instrumentation, not in IR.
Oct 8 2018
I meant just merging the passes. In the sense that you have one pass class with one module run() method and one function run() method. The question then is whether you'll ever want to use different options for the different IRUnits.
Some comments inline. Does it make sense to merge the module and function passes into one? Is it really necessary to distinguish the passes?
See my latest comment. If there is no state to be kept, that state need not be on the exported interface.
Oct 4 2018
Actually I was asking this because as far as I can tell, asan doesn't track state across functions and modules. Which means there's no point in _retaining_ the state, i.e. you don't have to keep an instance of it around.
Well, it will be a bit tricky to avoid moving the interfaces around, since AddressSanitizer contains quite a load of state
Is this the right place in the pipeline to put the passes? The legacy PM inserts the asan passes at EP_OptimizerLast, why not do the same thing?
Oct 1 2018
Thank you for moving this along!
Sep 27 2018
LGTM! Some final feedback by @chandlerc would be good though before landing :)
Sep 26 2018
Okay, all in all this looks alright, and making a single cleanup change seems less verbose than reverting rL340872 first. Please make very clear in this commit message that this is a partial revert and why.
Sep 25 2018
I like how simple this is becoming!
Sep 24 2018
Is that remaining change still useful? Does it make sense to revert instead?
is this in some sense a revert of rL340872? Which parts of that change remain?
LGTM, but I wonder why the PIC object is set up and used only in this change. Did we miss that in the change introducing it?
Sep 21 2018
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.
Sep 19 2018
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!
Sep 18 2018
Sep 15 2018
Sep 14 2018
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?
Sep 13 2018
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.
Sep 6 2018
I like how simple this is now!
Sep 5 2018
Here's what I think the ownership layering should look like:
Sep 3 2018
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. could be a viable alternative, the Passes library depends on IR anyways.
Instead of using a global timer registry, this should get a per-compilation registry from the PassExecutionCounter.
Aug 29 2018
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.