- User Since
- Feb 10 2017, 1:36 AM (132 w, 12 h)
Tue, Aug 13
Thanks for addressing this!
Fri, Aug 9
Which part of the basics specifically?
Also, avoid adding anything into the adaptor except just basic functionality of passing control to the target pass.
(say, as I commented already - skip remarks stuff altogether). This commit better contain the general comments on what MachineFunction is
and basics required for PassManager/Adaptor to start working, and nothing else.
Since you will have to add PassInstrumentation interfaces in order for PassManager to operate you might be able to add its usage into PassAdaptor as well.
But that could be done as a follow up change.
Okey, everything you said there makes perfect sense. Thanks for clarification.
Yep, you are right, standard instrumentation needs to be passed into PassBuilder for printing and time-passes to work.
Main changes are fine, just a small note on the test below.
Wed, Aug 7
add parameter to MemoryDependenceAnalysis constructor, clang-format
Had a chat with @ppf and there is a feeling that we are getting too many ways of customizing the pass (global command line options, explicit arguments passed to constructors, command line parsing etc).
If there is a need to customize passes/analyzes in a way similar to this patch then it should better be done by introducing a generic notion of PassSettings, handled in somewhat general manner through PassManager/PassBuilder.
Tue, Aug 6
( D65805 is not really a parent, just showing parts of the changes supporting my usecase )
and yes, this is more of a request for feedback than anything else...
Wed, Jul 31
Please, add a meaningful commit message that describes in high-level terms what/why you are doing here.
Tue, Jul 30
I believe in order to make progress here we first need to settle down on a very principal matter -
what kind of IRUnit MachineFunction is?
This definitely needs unit tests.
Please, check llvm/unittests/IR/PassManagerTest.cpp for Function/Module unit tests, and, say, llvm/unittest/Analysis/LoopPassManagerTest.cpp for Loop unit tests.
I took the liberty of setting a parent revision, hopefully thats what you did intend to do...
I'm not familiar with MIR printing usecases, and there are no tests for MIR printer.
Yet if MIR printing pass is to be applied to the Module only then there is no need to make it a MachineFunction pass, just do everything in a single run(Module...) method.
Jul 2 2019
Jun 28 2019
Thanks for starting this effort!
Jun 27 2019
Jun 26 2019
An example of inline-cost usage for the purpose of "inline body investigation" on inlining validity is in SampleProfileLoader::inlineCallInstruction.
Comments from it:
// Checks if there is anything in the reachable portion of the callee at // this callsite that makes this inlining potentially illegal.
It is interesting that initial version of D37779 that introduced this usage was initially setting "kinda infinite" Threshold in order to reach the same effect.
The root cause for what?
Jun 24 2019
Please, see my comment to PR42084: https://bugs.llvm.org/show_bug.cgi?id=42084#c3.
A core of the problem is in two difference Cost-vs-Threshold checks used in analyzeBlock (Cost >= Threshold) and analyzeCall (Cost < max(1, Threshold)).
I believe a proper fix for this bug would be to use a unified Cost-vs-Threshold check everywhere.
Jun 21 2019
Jun 17 2019
We already have the cost difference depending on this flag.
It seems to be rather obvious consequence of "full inline cost" notion and the difference should not be confusing...
Jun 13 2019
Btw, please, take a look at this review: https://reviews.llvm.org/D60740
I have not found time to integrate it yet, but it slightly changes the approach on handling this bonus
(main idea is to apply it to threshold, like majority of other bonues, instead of applying it to cost).
May 8 2019
discovered an unfinished review that Sanjoy was doing to address PR31181: D30446
(mentioning just in case it appears to be useful)
Apr 30 2019
Apr 27 2019
Apr 23 2019
Apr 22 2019
Apr 21 2019
Apr 18 2019
see D60870 for a proof-of-concept implementation of PipelineParser.
This is not yet complete (in particular parseAA is still in PassBuilder atm).
However I would like to get a feedback on overall organization.
- PipelineParser is a nested class in PassBuilder
- it is created for a single text-pipeline-parsing query
- text is processed into a pipeline vector and kept in the parser
- callbacks still reside in pass builder, accessed through a reference to pass builder kept in the parser
- callbacks take both Parser and array of Elements
Apr 17 2019
It might be a bit conceptually cleaner, but other than renaming the object from PassBuilder to PassPipelineParser (and internally talking to PassBuilder), what would be the API difference
compared to what is being suggested here?
I see a bunch of questions, e.g. - should we keep the callbacks in the parser object? etc
And I dont really see what practical benefits would we get from separating the parsing API out...
Right. We are building our own "kinda-SCC" pass manager, which runs inlining-devirtualization iteration doing walk over functions in rather different order than current SCC does.
This pass manager right now accepts at least one function pass pipeline (and we think about adding one more), and I would like to be able to populate this pass manager from -passes.
Apr 16 2019
New pass manager provides proper organization of analyses but it cant (and shouldnt!) hide innate asymmetry in relationship between Function-level analysis and Loop-level transformation.
NPM specifically prohibits reruns of outer-level analysis from within a inner-level-pass (proxy is readonly).
When working on loop-level you can only *use* function/module-level analysis, manually *preserve* it or automatically invalidate analysis results as a whole after the changes to IR.
That seems to be a proper design decision.
Apr 15 2019
moving NFC part out of this change
Apr 14 2019
Apr 12 2019
Apr 9 2019
Apr 8 2019
Apr 1 2019
Landed the change back, with fixes to module.modulemap. r357361.
Mar 31 2019
Mar 28 2019
just to make the comments visible :)
Pushed this through our java-based fuzzer, no problems found so far
(though our use of SCC-based passes is very limited).