Change the phase ordering of SROA in the LTO to enable more cse opportunities

[jinlin]

Summary

Change the phase ordering of SROA in the LTO to enable more cse opportunities.

[lebedev.ri]

This is only changing LTO side of obsolete pass manager.
If that is intentional, i'm not sure it's really worthwhile - it's obsolete pass manager is obsolete.

[jinlin]

In D105131#2847681, @lebedev.ri wrote:

This is only changing LTO side of obsolete pass manager.
If that is intentional, i'm not sure it's really worthwhile - it's obsolete pass manager is obsolete.

When I added -flto, this pass will be invoked. The LTO is used in the release build at Uber. Why do you think it is obsolete?

[lebedev.ri]

In D105131#2847819, @jinlin wrote:

In D105131#2847681, @lebedev.ri wrote:

This is only changing LTO side of obsolete pass manager.
If that is intentional, i'm not sure it's really worthwhile - it's obsolete pass manager is obsolete.

When I added -flto, this pass will be invoked. The LTO is used in the release build at Uber. Why do you think it is obsolete?

Because now the default is the New Pass Manager.

[jinlin]

In D105131#2847826, @lebedev.ri wrote:

In D105131#2847819, @jinlin wrote:

In D105131#2847681, @lebedev.ri wrote:

This is only changing LTO side of obsolete pass manager.
If that is intentional, i'm not sure it's really worthwhile - it's obsolete pass manager is obsolete.

When I added -flto, this pass will be invoked. The LTO is used in the release build at Uber. Why do you think it is obsolete?

Because now the default is the New Pass Manager.

Got you. The new pass manager is enabled by default starting from LLVM 12. I am using xcode 12.4 which uses prior version of llvm.

[fhahn]

In D105131#2847878, @jinlin wrote:

Got you. The new pass manager is enabled by default starting from LLVM 12. I am using xcode 12.4 which uses prior version of llvm.

The default of a specific Xcode version is not really relevant to upstream LLVM. If you build & use your own version of libLTO with Xcode's` ld, it would use the default of the upstream project anyways, right?

If this change has clear benefits, it should be used by both the new pass manager and the legacy one. Otherwise there may be additional regressions when the new pass manager is used.

To move forward with the patch, it would be good to include a phase-ordering test that shows the improvement of the new order. It would also be helpful if you could provide data on the impact in term of code-size and runtime performance on a larger set of programs/benchmarks to motivate the change.

[lebedev.ri]

I have three comments:

1. I don't think we should do any old-pm specific changes. If this applies to the new-pm, then sure, old-pm can be changed to keep it in sync.
2. I'm not sure why this is LTO-specific. Why should this not apply to the regular pipeline?
3. I'm extremely unconvinced that simply moving SROA run like it is done in the patch now is okay.

llvm/lib/Transforms/IPO/PassManagerBuilder.cpp

1084–1100

I may be reading this wrong, but to me it looks like we no longer do any SROA after running the inliner.
This function is called by populateLTOPassManager(), it does then call addLateLTOOptimizationPasses(),
but that one doesn't schedule any additional SROA runs.

llvm/lib/Transforms/IPO/PassManagerBuilder.cpp

void PassManagerBuilder::addLTOOptimizationPasses(legacy::PassManagerBase &PM) {
   PM.add(createWholeProgramDevirtPass(ExportSummary, nullptr));
 
   // That's all we need at opt level 1.
   if (OptLevel == 1)
     return;
 
   // Now that we internalized some globals, see if we can hack on them!
   PM.add(createGlobalOptimizerPass());
-  // Promote any localized global vars.
-  PM.add(createPromoteMemoryToRegisterPass());
+  // Break up allocas
+  PM.add(createSROAPass());
+  PM.add(createEarlyCSEPass());
 
   // Linking modules together can lead to duplicated global constants, only
   // keep one copy of each constant.
   PM.add(createConstantMergePass());
 
   // Remove unused arguments from functions.
   PM.add(createDeadArgEliminationPass());
 
void PassManagerBuilder::addLTOOptimizationPasses(legacy::PassManagerBase &PM) {
   if (AttributorRun & AttributorRunOption::CGSCC)
     PM.add(createAttributorCGSCCLegacyPass());
 
   // Try to perform OpenMP specific optimizations. This is a (quick!) no-op if
   // there are no OpenMP runtime calls present in the module.
   if (OptLevel > 1)
     PM.add(createOpenMPOptCGSCCLegacyPass());
 
   // Optimize globals again if we ran the inliner.
   if (RunInliner)
     PM.add(createGlobalOptimizerPass());
   PM.add(createGlobalDCEPass()); // Remove dead functions.
 
   // If we didn't decide to inline a function, check to see if we can
   // transform it to pass arguments by value instead of by reference.
   PM.add(createArgumentPromotionPass());
 
   // The IPO passes may leave cruft around.  Clean up after them.
   PM.add(createInstructionCombiningPass());
   addExtensionsToPM(EP_Peephole, PM);
   PM.add(createJumpThreadingPass(/*FreezeSelectCond*/ true));
 
-  // Break up allocas
-  PM.add(createSROAPass());
-
   // LTO provides additional opportunities for tailcall elimination due to
   // link-time inlining, and visibility of nocapture attribute.
   if (OptLevel > 1)
     PM.add(createTailCallEliminationPass());
 
   // Infer attributes on declarations, call sites, arguments, etc.
   PM.add(createPostOrderFunctionAttrsLegacyPass()); // Add nocapture.
   // Run a few AA driven optimizations here and now, to cleanup the code.