Page MenuHomePhabricator

Refactor identification of reductions and expose them as utility functions
AbandonedPublic

Authored by karthikthecool on Apr 15 2015, 10:31 PM.

Details

Summary

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

Diff Detail

Event Timeline

karthikthecool retitled this revision from to Refactor identification of reductions and expose them as utility functions .
karthikthecool updated this object.
karthikthecool edited the test plan for this revision. (Show Details)
karthikthecool set the repository for this revision to rL LLVM.
karthikthecool added a subscriber: Unknown Object (MLST).
jmolloy requested changes to this revision.Apr 16 2015, 3:40 AM
jmolloy added a reviewer: jmolloy.
jmolloy added a subscriber: jmolloy.

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

This revision now requires changes to proceed.Apr 16 2015, 3:40 AM
karthikthecool edited edge metadata.
karthikthecool removed rL LLVM as the repository for this revision.

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

rengolin edited edge metadata.Apr 16 2015, 5:47 AM

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.

karthikthecool edited edge metadata.

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

Will wait for your/James input on if to move the enums into namespace/structure.

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

anemet added inline comments.Apr 16 2015, 10:28 AM
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.

rengolin added inline comments.Apr 16 2015, 10:41 AM
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-

  1. Moved Enums inside structs.
  2. Marked local functions in struct as static.
  3. Moved functions inside of ReductionDescriptor.

No functionality change/No Regressions observed.
Please let me know your comments on the same.

Thanks and Regards
Karthik Bhat

rengolin added inline comments.Apr 17 2015, 7:54 AM
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

anemet edited edge metadata.Apr 17 2015, 3:25 PM

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

karthikthecool edited edge metadata.

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

rengolin accepted this revision.Apr 18 2015, 8:15 AM
rengolin edited edge metadata.

Hi Karthik,

Thanks for the extended changes, I know it took a while, but it looks a lot better now, thanks!

LGTM.

cheers,
--renato

karthikthecool accepted this revision.Apr 19 2015, 9:44 PM
karthikthecool added a reviewer: karthikthecool.

Thanks Renato. Comitted as r235284.

karthikthecool abandoned this revision.Apr 21 2015, 5:23 AM

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... :)