This is an archive of the discontinued LLVM Phabricator instance.

[ORE] Port TailRecursionElimination to the new API
ClosedPublic

Authored by davide on Jul 18 2017, 9:55 AM.

Details

Summary

I still need to add tests, but I wanted to make sure first we agree this is the right way of using the new API.
Unfortunately, no tests fail if I remove the calls to the old API.
This is https://bugs.llvm.org/show_bug.cgi?id=33788

Diff Detail

Repository
rL LLVM

Event Timeline

davide created this revision.Jul 18 2017, 9:55 AM
anemet edited edge metadata.Jul 18 2017, 10:07 AM

Yep, looks good.

lib/Transforms/Scalar/TailRecursionElimination.cpp
70 ↗(On Diff #107123)

Remove this?

305 ↗(On Diff #107123)

It's up to you but you may want to differentiate the remarks by name (i.e. not use the same "tailcalelim" name). It's easier to do stats, etc.

lenary added a subscriber: lenary.Jul 18 2017, 9:26 PM

Like I said in the other thread, you'll have a failing test in test/Other/new-pm-thinlto-defaults.ll I expect, I did.

davide updated this revision to Diff 107359.Jul 19 2017, 1:12 PM

@anemet, this should be ready for review.

davide added inline comments.Jul 19 2017, 1:17 PM
lib/Transforms/Scalar/TailRecursionElimination.cpp
70 ↗(On Diff #107123)

Which one in particular? The build fails without OptimizationDiagnosticInfo.h included.

305 ↗(On Diff #107123)

What do you mean exactly? Change the second parameter to be an unique identifier *within* the same file?
e.g. "tailcallelim" vs "tailcallreadnone"?
Fine, but please note that in this case it's less interesting as we're always applying the same transformation (i.e. set TailCall()).
In other passes, e.g. Value Numbering it may have more sense as we perform different kind of xforms (e.g. instruction removal, moving etc..)

davide added inline comments.Jul 19 2017, 1:19 PM
lib/Transforms/Scalar/TailRecursionElimination.cpp
305 ↗(On Diff #107123)

Still probably makes sense to differentiate why, although that should be always uniquely identified by the string (I guess stats don't really want to grep in text messages, so, OK).

modocache added inline comments.Jul 19 2017, 1:54 PM
lib/Transforms/Scalar/TailRecursionElimination.cpp
304 ↗(On Diff #107359)

Is this comment addition intentional?

davide added inline comments.Jul 19 2017, 2:01 PM
lib/Transforms/Scalar/TailRecursionElimination.cpp
304 ↗(On Diff #107359)

No, slipped through the cracks. I'll remove.

anemet accepted this revision.Jul 19 2017, 2:05 PM

LGTM once we clear the question below. Thanks!

lib/Transforms/Scalar/TailRecursionElimination.cpp
70 ↗(On Diff #107123)

OK :(

305 ↗(On Diff #107123)

Yes, exactly. It allows to do stats based on the situation under we perform the optimization. I.e. if a case never triggers we don't want to pay for the analysis, etc.

Looks like you convinced that it's worth using different names but then you didn't make the change...

This revision is now accepted and ready to land.Jul 19 2017, 2:05 PM

LGTM once we clear the question below. Thanks!

No, I just uploaded a stale version :(

This revision was automatically updated to reflect the committed changes.

Addressed your comments & committed.

Thanks!