This is an archive of the discontinued LLVM Phabricator instance.

[ADCE] Refactoring for new functionality (NFC)
ClosedPublic

Authored by david2050 on Aug 2 2016, 9:44 PM.

Details

Summary

This is another refactoring to break up the one function into three logical components functions.
Another non-functional change before we start added in features.

Diff Detail

Event Timeline

david2050 updated this revision to Diff 66612.Aug 2 2016, 9:44 PM
david2050 retitled this revision from to [ADCE] Refactoring for new functionality (NFC).
david2050 updated this object.
david2050 added reviewers: nadav, mehdi_amini, majnemer.
david2050 added subscribers: llvm-commits, freik.
nadav added inline comments.Aug 3 2016, 10:10 AM
lib/Transforms/Scalar/ADCE.cpp
51

Please use doxygen style comments (three slashes) and a period at the end of the sentence. Please do the same for the other comments above.

Also, please name the parameter. If the name does not give a good indication of what the parameter is you can use '\p' to describe the parameter.,

68

Doxygen, period, just like above.

76–79

Typo here. 'gig'

david2050 updated this revision to Diff 66676.Aug 3 2016, 10:25 AM

Doxygen style comments with periods. Fix spelling of Aggressive

twoh added a subscriber: twoh.Aug 3 2016, 10:27 AM
mehdi_amini added inline comments.Aug 3 2016, 10:29 AM
lib/Transforms/Scalar/ADCE.cpp
51

Also, just a nit, I'm used to to read Return true for... instead of True for.

majnemer added inline comments.Aug 3 2016, 10:30 AM
lib/Transforms/Scalar/ADCE.cpp
92

Would it make sense to use !isInstructionTriviallyDead here instead?

93–95

Perhaps the logic for profile instrumentation can be sunk into isInstructionTriviallyDead?

david2050 updated this revision to Diff 66679.Aug 3 2016, 10:50 AM

Save David's suggestions. Add parameter name

david2050 added inline comments.Aug 5 2016, 10:04 AM
lib/Transforms/Scalar/ADCE.cpp
93–95

I tried to reply via email but it got lost in space. I prefer to not make this change until I get the my changes landed just for stability of baseline. But I will get to to it before we are done.

majnemer accepted this revision.Aug 5 2016, 10:14 AM
majnemer edited edge metadata.

LGTM

lib/Transforms/Scalar/ADCE.cpp
93–95

Sure, sounds reasonable to me.

This revision is now accepted and ready to land.Aug 5 2016, 10:14 AM
david2050 closed this revision.Aug 5 2016, 12:45 PM