Trying to add the plumbing necessary to add tuning options to the new pass manager.
Testing with the flags for loop vectorize.
Details
Diff Detail
- Repository
- rL LLVM
- Build Status
Buildable 30749 Build 30748: arc lint + arc unit
Event Timeline
include/llvm/Passes/PassBuilder.h | ||
---|---|---|
69 | Maybe: "Tunable parameters for passes in the default pipelines." | |
70 | Especially in the type name I'd like to emphasize that this isn't really for fundamental options, but for tuning. Based on my comment below about how to use this, I would suggest: PipelineTuningOptions. | |
72 | Make this a doxygen comment and add more details? Assume the reader has just started trying to use this from a library like Halide. What do they need to know? | |
74–75 | For consistency and readability, let's use positive bools and just change the defaults? | |
74–75 | Vertical whitespace and doxygen comments on each option since these are expected to be directly controlled by users? | |
93 | Maybe PTO? | |
210 | Given that the "none" behavior is to just use the default constructor, I'd rather just accept by value and have the default argument be the default constructed value. That will in turn let us trivially initialize things. | |
lib/Passes/PassBuilder.cpp | ||
223 | Why not support a CLI here? | |
866 | If I understand correctly, the first parameter isn't about *unrolling* but about interleaving. We should be careful to use that terminology as disabling unrolling is a potentially reasonable thing to add. | |
lib/Passes/PassRegistry.def | ||
201 ↗ | (On Diff #191964) | I wouldn't use the tuning options here. If we want to support explicitly controlled behavior of the vectorization pass here, we should do so by *parsing* values out of the textual pipeline description much as we do for a few other passes. The tuning struct seems more appropriate for the pipeline above exclusively (and I would name it accordingly). |
lib/Passes/PassBuilder.cpp | ||
---|---|---|
866 | @chandlerc, you are right about the terminology mess. It is the vectorizer that is "incorrectly" piggybacking on the unroll flag. If we are to start fixing the terminology here, we'd need to create DisableInterleaveLoops. If we are going to that direction, I suggest starting from setting the same value as DisableUnrollLoops so that fixing the terminology mess is NFC. Potentially bigger mess is programmer education, to teach that interleaving and unrolling are different, as they often tend to result in identical ASM after instruction reordering --- but I fully agree that we still need to educate them (including compiler writers) to use the appropriate terminology. |
lib/Passes/PassBuilder.cpp | ||
---|---|---|
866 | Yes, that's true. I used the same naming and values as the ones in the old pass manager (including not having a cl option). Updated now (also means the old and new pass managers are now inconsistent as far as namings, defaults etc, but it's probably preferable to update the old one to be consistent with this?). | |
lib/Passes/PassRegistry.def | ||
201 ↗ | (On Diff #191964) | I removed the options for now, but I'm still confused how things work TBH. What if the defaults in the cl::opts are different from the constructor defaults? Regarding parsing, I tried to understand how this is done for LoopUnroll and I only got more confused. The tuning I had in mind to add next was -max-acc-licm-promotion in LICM. Ideally, I'd like that the value of this flag be used always as the default. When this is changed with the flag, use that new default. When it's changed in the PassBuilder, again, use that new default. Whereas if I add the parsing approach here, there's a "second source of a default". I'd also prefer to not have the defaults embedded in the constructor, because, again, that's a "second source of defaults". If we later decide to change the default in the cl::opt option, then we also need to change the default in the constructor, no? (I am thinking of unsigned values here, caps, not bools that enable/disable things that are less likely to change often) I still haven't understood how everything here works, so any kind of clarifications are very much welcome! |
Attempting to merge defaults to a single cl::opt.
Please note the comments on defaults in lib/Transforms/Vectorize/LoopVectorize.cpp. Feedback there very welcome.
Details:
The default for LoopVectorize in the PassManagerBuilder is false. (opt checks for this, and sets LoopVectorize to true based on Olevel/size)
The default when creating the LoopVectorize with no arguments, is true (VectorizeOnlyWhenForced = false).
I think the approach here looks really good. Comments below.
include/llvm/Passes/PassBuilder.h | ||
---|---|---|
77 | Comment needs updating | |
77–81 | Comment needs updating. | |
78–82 | I think we should name these consistently. Two options that seem equally good to me: InterleaveLoops and VectorizeLoops or LoopInterleaving and LoopVectorization I think I very mildly lean toward the second, but it is a very mild preference. | |
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
280 | You should be able to just define this as llvm::SetLoopsInterleaved? | |
282–285 | Following whatever pattern you pick above for the PassTuningOptions, I'd call these EnableFooLoops or EnableLoopFoo for both interleaving and vectorization. | |
283 | Update description | |
290–295 | Not really sure what this commenting structure is trying to convey, but I don't think you want to use commented out code. I'd just write prose in a comment on the method that describes what's going on here. |
lib/Passes/PassBuilder.cpp | ||
---|---|---|
866 | I think Alina's patch does a reasonable job of starting to fix this. I don't think the education problem is that bad. Unrolling and interleaving produce pretty different results in LLVM. |
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
290–295 | I *really* don't want to leave this as is. It was meant as a discussion point. Sorry for not being clearer. I wanted to use the "expected defaults", where interleaving and vectorization are enabled by default, but vectorization is disabled by default in the old pass manager. I guess I am asking why are having vectorization disabled by default in the old pass manager, but enabled by default when creating the LoopVectorizePass with no params? We also have a strange (to me) behavior in opt.cpp:351:
So I can't really change the default to enable vectorization, without some changes here. |
Address comments.
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
290–295 | To make progress here, I moved the explanation to a comment block. |
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
290–295 |
Quite confusingly, clang enables loop vectorization in the legacy pipeline builder by default again. That is, opt -O3 disables LoopVectorize, but clang -O3 has it enabled. |
LGTM with comments below addressed. All pretty minor.
include/llvm/Transforms/Vectorize/LoopVectorize.h | ||
---|---|---|
92–100 | I'd move this to be a doxygen comment for the method -- people are likely to want to understand this when using the default constructor. | |
102 | I'd name the variables exactly the same as the members. It'll also help people use descriptive comments when calling it. | |
lib/Transforms/Vectorize/LoopVectorize.cpp | ||
287–288 | stray commented out bit left over here... | |
290–295 | FWIW, all of this makes "sense" now to me. I'm pretty happy w/ you changing the defaults to be more consistent Alina, but like your approach of making that a separate patch. And yeah, the fact that clang and opt have different defaults I think is just because we didn't wire things together in a way that would make them line up easily the first time around. |
Maybe: "Tunable parameters for passes in the default pipelines."