This is an archive of the discontinued LLVM Phabricator instance.

[TRE][NFC] Refactor shared state into member variables.
ClosedPublic

Authored by laytonio on May 2 2020, 5:04 PM.

Details

Summary

Separate functions that require shared state into a class to avoid needing to pass them though multiple functions just to be available where needed.

The main motivation for this is that we would like to remove the limitation that accumulator values be dynamic constant, which would require additional shared state between call eliminations in the same function, compounding this issue.

Diff Detail

Event Timeline

laytonio created this revision.May 2 2020, 5:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2020, 5:04 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

Please make the commit message describe what this patch is doing, as opposed on what you plan to do as a followup. (It's okay to briefly mention the followup, but the commit should stand on its own for anyone reading it in the future.)

We usually prefer to put functions that are more than a couple lines outside the class. The benefit is mostly just to reduce the indentation.

llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp
447–448

Please make sure this class is in an anonymous namespace.

456

I'd prefer not to keep around mutable state like this in the class... but if you are, it needs better comments explaining what it represents and where it gets modified.

In particular, I was trying to work out what TailCallsAreMarkedTail represents, and ended up looking in circles.

laytonio updated this revision to Diff 261960.May 4 2020, 4:48 PM
laytonio retitled this revision from [TRE][NFC] Refactor in preparation for removal of isDynamicConstant to [TRE][NFC] Refactor shared state into member variables..
laytonio edited the summary of this revision. (Show Details)

Added anonymous namespace. Renamed two variables to be more clear. Added some comments.

laytonio marked an inline comment as done.May 4 2020, 4:50 PM
laytonio marked an inline comment as done.May 4 2020, 4:56 PM

In general I agree that keeping mutable state in the class like this is not optimal. In this case though, I do believe it is better than needing to trace the variable up though several functions to find were it is declared, when in is only used in the bottom few functions. I added a comment stating that these variables are populated in createTailRecurseLoopHeader to hopefully make them a little easier to understand. As for moving the functions out of the class, I was trying to avoid cluttering the code with a bunch of "TRE->" everywhere, but if we think it is cleaner to have this be a struct that is passes around instead of a class with member functions, I am okay with that.

By "move the definitions", I just meant leave a forward declaration in the class definition, and the function definition outside the class definition.

Otherwise makes sense.

laytonio updated this revision to Diff 262211.May 5 2020, 1:43 PM

Ah, I misunderstood, fixed now.

laytonio updated this revision to Diff 262218.May 5 2020, 1:53 PM

Whoops, deleted one to many static keywords.

laytonio updated this revision to Diff 262219.May 5 2020, 1:56 PM

Nevermind, I'm an idiot.

efriedma accepted this revision.May 5 2020, 6:38 PM

LGTM

This revision is now accepted and ready to land.May 5 2020, 6:38 PM

I'll commit this for you. How do you want to be credited in the "Author" line of the git commit?

Name and email would be great. Thank you.

This revision was automatically updated to reflect the committed changes.