Page MenuHomePhabricator

Rework of the interface to enable shrink wrapping
ClosedPublic

Authored by kbarton on Aug 24 2015, 11:10 AM.

Details

Reviewers
qcolombet
hfinkel
Summary

Based on comments from Hal (http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20150810/292978.html), I've changed the interface to add a callback mechanism to the TargetFrameLowering class to query whether the specific target supports shrink wrapping. By default, shrink wrapping is disabled by default. Each target can override the default behaviour using the TargetFrameLowering::targetSupportsShrinkWrapping() method. Shrink wrapping can still be explicitly enabled or disabled from the command line, using the existing -enable-shrink-wrap=<true|false> option.

Diff Detail

Event Timeline

kbarton updated this revision to Diff 32980.Aug 24 2015, 11:10 AM
kbarton retitled this revision from to Rework of the interface to enable shrink wrapping.
kbarton updated this object.
kbarton added reviewers: hfinkel, qcolombet.
kbarton added subscribers: llvm-commits, wschmidt.
kbarton updated this revision to Diff 32990.Aug 24 2015, 1:27 PM

Pass the MachineFunction to the targetSupportsShrinkWrapping method, to allow function-specific decisions to be made about whether to shrink-wrap it. PPC needs to make this decision based on the ABI and 32/64 bit.

qcolombet accepted this revision.Aug 25 2015, 3:19 PM
qcolombet edited edge metadata.

Hi Kit,

Small nitpicks inlined but LGTM otherwise.

Thanks,
-Quentin

include/llvm/Target/TargetFrameLowering.h
144

Do not repeat the method name in the comment.

146

Maybe enableShrinkWrapping?
“Supports" is kind of misleading because for instance X86 supports it, but it does not enabled it by default yet.

(Personally, I would not repeat target in the method name, but I see that are precedence :).)

lib/CodeGen/ShrinkWrap.cpp
155

Period at the end of the comment.

156

Could be a static method.

332

The formatting seems weird here.

332

Maybe put this check with the previous condition, i.e., if MF.empty || !isShrinkWrapEnabled(MF).

This revision is now accepted and ready to land.Aug 25 2015, 3:19 PM
hfinkel edited edge metadata.Aug 27 2015, 8:12 PM

Please take care of Quentin's "nitpicks", otherwise, LGTM too. Thanks!

include/llvm/Target/TargetFrameLowering.h
146

I agree, enableShrinkWrapping seems good.

kbarton marked 7 inline comments as done.Aug 31 2015, 10:37 AM

Addressed Quentin's comments. I"ll commit this shortly.

lib/CodeGen/ShrinkWrap.cpp
156

I've made it static, and dropped the const qualifier as a result.

kbarton closed this revision.Aug 31 2015, 11:28 AM
kbarton marked an inline comment as done.

Committed revision 246463.