Page MenuHomePhabricator

Break LoopUtils into an Analysis file.
ClosedPublic

Authored by tvvikram on Aug 22 2018, 11:15 PM.

Details

Summary

The InductionDescriptor and RecurrenceDescriptor classes basically analyze the IR to identify the respective IVs. So, it is better to have them in the "Analysis" directory instead of the "Transforms" directory.

The rationale for this is to make the Induction and Recurrence descriptor classes available for analysis passes. Currently including them in an analysis pass produces link error (http://lists.llvm.org/pipermail/llvm-dev/2018-July/124456.html).

Induction and Recurrence descriptors are moved from Transforms/Utils/LoopUtils.h|cpp to Analysis/IVDescriptors.h|cpp.

Diff Detail

Repository
rL LLVM

Event Timeline

tvvikram created this revision.Aug 22 2018, 11:15 PM

A gentle ping! Can someone give few basic thoughts about this change?

Thanks,
Vikram

Hello

These classes to me look large enough to warrant their own file. Maybe something like RecurrenceDescriptor.h/cpp? Or a name that captures InductionDescriptor too? I'm not sure what that would be though..

I'd also recommend splitting the transformation parts out into other functions we you can, then the Base's from here can just become the standard classes.

llvm/Transforms/Utils/LoopUtils.h
77 ↗(On Diff #162137)

As this function is static, how about turning it into a free standing function and removing RecurrenceDescriptor (renaming RecurrenceDescriptorBase to RecurrenceDescriptor)

93 ↗(On Diff #162137)

This is only used in the vectoriser? Perhaps again make it stand alone (perhaps in LoopVectoriser.cpp if it fits better there).

Hi @dmgreen,

Thank you for your inputs.

I think having both the descriptor classes in a single file would be sufficient; maybe under a file named IVDescriptors.h/cpp? Please do let me know.
Meanwhile, I will move the functions that do the transformations out of the descriptor classes and resubmit the patch.

Thanks,
Vikram

dmgreen added a comment.EditedSep 9 2018, 1:23 PM

Yeah, IVDescriptors sounds like a good name to me. Thanks for making this cleaner!

tvvikram updated this revision to Diff 164793.Sep 10 2018, 8:48 PM

Moved Induction and Recurrence descriptors from LoopUtils.h/cpp to IVDescriptors.h/cpp.

tvvikram edited the summary of this revision. (Show Details)Sep 10 2018, 8:53 PM
dmgreen accepted this revision.Sep 11 2018, 12:27 PM

Yep, LGTM!

This revision is now accepted and ready to land.Sep 11 2018, 12:27 PM
This revision was automatically updated to reflect the committed changes.

Thank you @dmgreen for the review!