[PATCH] D147954: [SimplifyCFG]Prevent premature simplification of indirect callsites with profile data

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Instead of making this false by default and requiring it to be set from the command line for pre-LTO compile steps, it would be better to add a "setPreserveIndirectCallInstWithProfInHoistAndSink" interface and set this to false when appropriate from the pass pipeline builder (see other examples where the SimplifyCFGOptions are set from there). That way it can be controlled more exactly. I'm a little confused why we would disable this only in the pre-LTO compile step right now: I suppose this is specific to SamplePGO because ICP is not performed during the pre-LTO compile step? In other cases (non-LTO and instrumentation PGO) ICP appears to be performed right after profile annotation.

However, we also perform SimplifyCFG at least once before profile annotation (both sample and instrumentation PGO). Wouldn't we still end up merging indirect calls in some cases too aggressively, because they don't have any profile metadata so we won't know any better? I'm currently testing a change to avoid speculation in SimplifyCFG before profile annotation to avoid doing incorrectly aggressive transformations. We might want to do something similar here too: setPreserveIndirectCallInstWithProfInHoistAndSink(false) for all pre-PGO annotation invocations of SimplifyCFG in PGO compiles.



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1817
 
-    // Conservatively return false if I is an inline-asm instruction. Sinking
-    // and merging inline-asm instructions can potentially create arguments
-    // that cannot satisfy the inline-asm constraints.
-    // If the instruction has nomerge or convergent attribute, return false.
-    if (const auto *C = dyn_cast<CallBase>(I))
+    if (const auto *C = dyn_cast<CallBase>(I)) {
+      // Conservatively return false if I is an inline-asm instruction. Sinking
----------------
Suggest undoing this change which just moves the comment a little but unnecessarily adds a diff.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147954/new/

https://reviews.llvm.org/D147954