- User Since
- Feb 10 2017, 1:36 AM (123 w, 4 d)
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.
Fri, Jun 21
Mon, Jun 17
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...
Thu, Jun 13
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).
Mar 26 2019
Please, update verify-safepoint tests to use new-pass-manager flavor.
Mar 22 2019
Mar 20 2019
moved legacy test code around to remove the need for additional namespace manipulations
Mar 17 2019
changing status to reflect that there is no active work on this patch atm.
Mar 15 2019
I dont quite understand what clang is doing in terms of timers (and what -disable-free is/how does it interact with -ftime-report).
However if after doing printAll() you expect that there wont be any other reports then you should call clearAll() immediately after.
Thanks for yet another thorough review!
removing unneeded assert
test cleanup - removing unneeded stuff, adding a few more checks, adding comments
Thanks for review!
Mar 14 2019
handling default case the same way as it was before;
adding unit-test for non-default case
Okey, a proper solution here is to get back to not setting the stream by default and restort to old CreateInfoOutputFile-just-for-printing scheme.
This way we do not need to play with ownership of that info-output-file stream.
Okey, let me enumerate all the possible solutions that I see:
- own stream in TimePassesHandler (my initial implementation)
- create TimePasses disabled/no-stream, enable from NewPMDriver (current state), own by NewPMDriver
- pass the stream to StandardInstrumentation constructor, pass through into TimePassesHandler constructor, own by NewPMDriver
- change behavior of CreateInfoOutputFile and make it handle the ownership itself
That is purely a chicken/egg issue.
CreateInfoOutputFile creates a new stream and unless we retake the ownership it will be destroyed.
outstream in TimePasses is set when enabling, so now it is not possible
to enable and have outstream == nullptr.
As per discussion with @philip.pfaffe - owning output stream is rather strange for TimePasses object.
This ownership idea came from CreateInfoOutputFile design, as it returns new stream each time.
Abandoning this in favor of D59366.
Feb 27 2019
Agreed with @rsmith, running 32 times and collecting the amount of successes == 16 should be a fine way to do this.
I do like the final result!
See my comment, but its good to go even w/o my suggestion.
Do you suggest to get rid of the current scheme where we have a single output file for all kinds of "info data"?
Say, now w/o these changes -info-output-file is controlling the output file for both -stats and -time-passes.
Do you think it was a wrong design and we need to introduce separate output controls for each of those features?