Page MenuHomePhabricator

Remove the ScalarReplAggregates pass
ClosedPublic

Authored by majnemer on Jun 13 2016, 5:05 PM.

Details

Summary

Nearly all the changes to this pass have been done while maintaining and
updating other parts of LLVM. LLVM has had another pass, SROA, which
has superseded ScalarReplAggregates for quite some time.

Diff Detail

Repository
rL LLVM

Event Timeline

majnemer updated this revision to Diff 60630.Jun 13 2016, 5:05 PM
majnemer retitled this revision from to Remove the ScalarReplAggregates pass.
majnemer updated this object.
majnemer added a subscriber: llvm-commits.
whitequark edited edge metadata.Jun 13 2016, 5:17 PM

This is great!

lib/Transforms/IPO/PassManagerBuilder.cpp
205 ↗(On Diff #60630)

Indentation? (Also below)

majnemer added inline comments.Jun 13 2016, 7:22 PM
lib/Transforms/IPO/PassManagerBuilder.cpp
205 ↗(On Diff #60630)

The indentation is fine, phab is just not showing the whitespace change.

silvas added a subscriber: silvas.Jun 13 2016, 7:59 PM

Does SROA pass all the test cases for ScalarRepl? I remember when Davide was looking at IPCP vs IPSCCP (with IPCP seeming to be dead) that there were actually some cases missed by IPSCCP and so IPCP wasn't deleted.

Does SROA pass all the test cases for ScalarRepl? I remember when Davide was looking at IPCP vs IPSCCP (with IPCP seeming to be dead) that there were actually some cases missed by IPSCCP and so IPCP wasn't deleted.

A partial survey indicates that SROA does all that ScalarRepl can do. Some ScalarRepl tests which fail are grep based and thus sensitive to the exact IR produced by the pass, others fail because SROA produces IR with fewer bitcasts (ScalarRepl produces stuff like trunc i160 undef to i32).

In any case, it's pretty moot: nobody uses ScalarRepl which makes it effectively dead.

karthikthecool added inline comments.
lib/Transforms/Scalar/Scalar.cpp
205 ↗(On Diff #60630)

May be we can remove this function as this is same as LLVMAddScalarReplAggregatesPass ?

208 ↗(On Diff #60630)

May be we can remove this function as well as we are not using the Threshold and this is same as LLVMAddScalarReplAggregatesPass ?

@karthikthecool
I acknowledge that we could remove those functions but our developer policy for the C API recommends that we take a best-effort approach: http://llvm.org/docs/DeveloperPolicy.html#c-api-changes

I figure that keeping the functions around is a net win.

chandlerc accepted this revision.Jun 14 2016, 12:30 AM
chandlerc added a reviewer: chandlerc.
chandlerc added a subscriber: chandlerc.

LGTM. I'm a bit sad to not kill this myself, but you should just kill it with fire while you're here. You've done the hard deletion work. I'm glad to see it go now that there is no real reason to keep it around.

(And FWIW, the two things that held it around for this long were the C API and before SROA switched to always use the DomTree some hypothetical concerns about compile time impact of the SSAUpdater path with the new SROA being worse than with ScalarRepl. The latter has been obviated, and the former seems well addressed here and with the C-API policy changes.)

This revision is now accepted and ready to land.Jun 14 2016, 12:30 AM

Finally! Yes! Thank you!

(And LGTM!)

This revision was automatically updated to reflect the committed changes.