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.
hfinkel lei nemanjai stefanp
- Group Reviewers
- rGcaa10988bef0: [PowerPC] Add options for PPC to enable/disable using non-volatile CR
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...
Well, this sounds like a tune option.I am fine with it. Just one concern on the implementation.
|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.
The remaining comments are stylistic nits that can be addressed on the commit. Assuming that those will be fixed, LGTM.
"Disable the use of non-volatile CR register fields."
Line too long?
The naming is odd here - you use No for the setter and Disabled for the getter. Please name them consistently. Perhaps setDisableNonVolatileCR() and isNonVolatileCRDisabled().
Please keep to 80 columns in .td files as well.