Page MenuHomePhabricator

Add options for PPC to enable/disable using non-volatile CR
ClosedPublic

Authored by NeHuang on Nov 4 2019, 5:30 PM.

Details

Summary

In this patch, an option is added for PowerPC to disable using non-volatile CR and avoid CR spilling in the prologue. According to performance analysis results, we found performance gain for a few SPEC2017 benchmarks without introducing significant degradation.

Diff Detail

Event Timeline

NeHuang created this revision.Nov 4 2019, 5:30 PM
jsji added a reviewer: Restricted Project.Nov 4 2019, 6:21 PM
Yi-Hong.Lyu added inline comments.
llvm/lib/Target/PowerPC/PPCRegisterInfo.td
368

Why do we need to check isELFv2ABI()?

@Yi-Hong.Lyu For PowerPC, CR2, CR3, CR4 are non-volatile CR fields according to ELFv2ABI, which will not hold for other ABIs.

I didn't take deep look at the detail. But I think, it is better to implement such with target feature, and enable it for specific cpu. Another thing is that, if it is positive overall, why disable it by default?

I only saw a minor nit with this patch overall.

To address the comment from the previous reviewer I don't feel we should enable this by default on a given CPU even if SPEC does not show significant degradations. This patch effectively tells the register allocator that it should never use the non-volatile CR registers if the option is given. If we make this the default we are effectively abandoning part of our hardware by telling the compiler never to use it. Anyway, that's my opinion on this...

llvm/test/CodeGen/PowerPC/ppc-disable-non-volatile-cr.ll
33

nit:
You can clean up the #2 from this line and the other two lines below.

Well, this sounds like a tune option.I am fine with it. Just one concern on the implementation.

llvm/lib/Target/PowerPC/PPCRegisterInfo.h
25 ↗(On Diff #227803)

This is not a good idea as you are polluting the namespace. If you really want to share information between compilation unit, you can declare local variable in sub target, and initialize it to the debug opt some where.

nemanjai requested changes to this revision.Nov 21 2019, 4:01 AM

This needs to be a property of PPCFunctionInfo and set when the option is provided.

This revision now requires changes to proceed.Nov 21 2019, 4:01 AM
NeHuang updated this revision to Diff 236438.EditedJan 6 2020, 12:42 PM

Addressed the comments by

  • Add "DisableNonVolatileCR" into "PPCFunctionInfo" as a new property, add the functions to set and check it.
  • Set the new member in the constructor based on the value of the option.
  • Clean up redundant "#2" in the test case.

@nemanjai @stefanp @steven.zhang

Yi-Hong.Lyu added inline comments.Jan 8 2020, 10:09 AM
llvm/lib/Target/PowerPC/PPCRegisterInfo.td
369

Why do you NOT introduce a temp variable in the former condition but introduce one in the later one? I would suggest choose one style and use that uniformly.

380

Same as above.

nemanjai accepted this revision.Jan 9 2020, 4:13 AM

The remaining comments are stylistic nits that can be addressed on the commit. Assuming that those will be fixed, LGTM.

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
18

"Disable the use of non-volatile CR register fields."

llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h
68

Line too long?

182

The naming is odd here - you use No for the setter and Disabled for the getter. Please name them consistently. Perhaps setDisableNonVolatileCR() and isNonVolatileCRDisabled().

llvm/lib/Target/PowerPC/PPCRegisterInfo.td
366

Please keep to 80 columns in .td files as well.

369

+1

This revision is now accepted and ready to land.Jan 9 2020, 4:13 AM
amyk added a subscriber: amyk.Jan 9 2020, 8:42 PM
amyk added inline comments.
llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h
68

s/Inidcates/Indicates

Also, end with a period.

This revision was automatically updated to reflect the committed changes.