Hi All,
I would like to reuse LoopVectorizer's code to identify/categorize reductions in one of my Pass.
Refactor functions/code required to identify a reduction and expose them as utility functions.
No functionality change.
Please let me know if this is good to commit.
Thanks and Regards
Karthik Bhat
Hi Karthik,
I like the general direction. We have reduction detection logic in too many places already (LoopReroller, I'm looking at you!) and this will enable us to only have one.
However, the functions you've ripped out of LoopVectorizationLegality no longer make sense to be in LoopVectorize.cpp - they should be in lib/Transforms/Utils to avoid a layering violation.
Cheers,
James
Hi James,
Thanks for the review. Yes it makes sense to move it out of LoopVectorizer.cpp. Updated the code as per reveiw comments.
Added a file LoopUtils.cpp in lib/Transforms/Utils with refactored code. We can add any additional common loop functionalities in this util file.
Please let me know if this looks good to you.
Thanks and Regards
Karthik Bhat
Hi Karthik,
This is looking a lot better, thanks!
I have two main comment inline, and I'm assuming it passes all check-all tests and maybe the test-suite in at least one major platform.
Also, please remember to add "NFC" (meaning non-functional-change) to your commit message, since this is just the refactoring part of your patch.
Finally, it'd be good to see how your pass looks like with this change, to make sure we won't need additional refactorings to this piece of code. Can you add a new diff to the loop interchange pass that assumes these changes are applied?
cheers,
--renato
| include/llvm/Transforms/Utils/LoopUtils.h | ||
|---|---|---|
| 47 | These enums now have llvm namespace. While RK clearly means "reduction kind" in the context of the loop vectorizer, it doesn't in a general context. I'm not sure what's best in this case, to add a namespace or move it inside some of the structs below. @jmolloy? | |
| lib/Transforms/Utils/LoopUtils.cpp | ||
| 195 | Change this comment here to state that the exit instruction will be saved as you create the reduction descriptor below. | |
Hi Renato,
Thanks for your valuable inputs. I have addressed one of your comments and modified the comments as per your suggestion.
Will wait for your/James input on if to move the enums into namespace/structure.
Yes I have run make check-all on X86 and there are no regressions.
Sure I will update the LoopInterchange pass assuming this change. We wont need any additional refactoring and we can reuse these functions as it is.
Thanks once again for your time and support.
Thanks & Regards
Karthik Bhat
Yes, and maybe @aschwaighofer, too.
Yes I have run make check-all on X86 and there are no regressions.
Good. Thanks!
Sure I will update the LoopInterchange pass assuming this change. We wont need any additional refactoring and we can reuse these functions as it is.
Excellent. Looking forward to seeing it.
cheers,
--renato
| include/llvm/Transforms/Utils/LoopUtils.h | ||
|---|---|---|
| 46–63 | I haven't dealt with this part of the vectorizer yet so I only have general comments about some interface improvements, now, that these interfaces are made public. Are these enums local to the structures that follow right after? If yes, they should be local to those structs. | |
| 124–127 | These are missing comments. Also if they operate on a ReductionDesc, they should probably made a member function. And possibly turning the struct into classes to describe the public interface of the class. | |
| 129–131 | These don't look like loop-related utilities. Are you using these in your pass? In that case you either want to move them to Instruction.h or make them static member of the above classes if these are specific to the algorithm finding reductions. | |
| include/llvm/Transforms/Utils/LoopUtils.h | ||
|---|---|---|
| 46–63 | Almost, but I think we can make it be. The only other function that uses it is createMinMaxOp() down there, but it could also be part of this structure/class. Then, moving this enum in there would be trivial. Same goes for ReductionKind and getReductionBinOp(). Internally, using the struct as context on external functions would also work. | |
| 124–127 | If there are no private members, there isn't much point in making it into a class. But I agree, missing comments. | |
| 129–131 | These were local functions to the loop vectorizer and loop interchange, and I agree they're very generic. Instruction.h seems like a better place. | |
Hi All,
Updated the code to address Adam,Renato's comments.
Following are the changes-
No functionality change/No Regressions observed.
Please let me know your comments on the same.
Thanks and Regards
Karthik Bhat
| include/llvm/Transforms/Utils/LoopUtils.h | ||
|---|---|---|
| 65 | Now that these structs are public, we may want to make them classes to hide these implementation details. Same for ReductionDescriptor. | |
| 137 | Probably most of these functions can be static. Only the ones operating on the internal memebers should be non-static. | |
| 152 | I don't think this is necessary, just use ReductionKind::RK_IntegerMinMax or even ReductionKind::IntegerMinMax. | |
| lib/Transforms/Vectorize/LoopVectorize.cpp | ||
| 2626–2627 | Keep the comparison with the enum value here. | |
| 2638 | this should be static function. | |
Hi Renato,
Thanks for the review. Addressing review comments.
I have made ReductionInstDesc and ReductionDescriptor as class as per your suggestion. Made member variables as private and exposed them through get functions so that they can be used in LoopVectorizer. I hope you are ok with this change?
Thanks for your time and awaiting for your inputs.
Regards
Karthik Bhat
I'll let Renato OK this, but there is one more thing I've noticed. Thanks.
| include/llvm/Transforms/Utils/LoopUtils.h | ||
|---|---|---|
| 19–21 | It does not look like you tried to minimize the included headers by relying on forward decls. See http://llvm.org/docs/CodingStandards.html#include-as-little-as-possible | |
Hi Adam,
Thanks for the link. It was quite useful. Updated the code to remove redundant #includes.
Please let me know if you have any other comments.
Thanks and Regards
Karthik Bhat
Hi Karthik,
Thanks for the extended changes, I know it took a while, but it looks a lot better now, thanks!
LGTM.
cheers,
--renato
This has been submitted as r235284.
Not getting an option to close revision hence abondoning it..:(
The option to close it in the drop-down menu of the comment section. I know, hidden... :)