[PATCH] D133999: [SelectOpti] Restrict load sinking

[Sotiris Apostolakis via Phabricator via llvm-commits]

apostolakis added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:713
+  // Ensure that the instructions are on the same basic block.
+  if (LoadI->getParent() != SinkPt->getParent())
+    return false;
----------------
davidxl wrote:
> In most of the cases, the load and sinkpt are not in the same bb right -- as the case in the test case I commented.
Actually, most of the sinkable loads are in the same basic block.
For a search workload, only 2.8% of the sinkable loads were in a different BB, while the rest (i.e., the vast majority, 97.2%) were in the same BB as the sinkpt.
The intuition here is that sinkable loads, as defined in this optimization, are loads that have a single-use (only used for the purpose of computing the select operand). Thus, it is expected that they will be close-by in the same BB as the select, i.e., their use.
If they are not in the same BB, there might be a good reason for that. Either there is a state-modifying and aliasing instruction in-between that prevents them for moving, or due to some other optimization (e.g., for both reported bugs, the loaded stack objects were freed early).

Note also that for the search workload, out of all the sinkable loads in the same BB, only 3 load-sinks (0.06% of sinkable loads) were conservatively skipped due to the check for potential (although non-aliasing in these cased) state-modifying instructions in line 717. So, even for same-BB loads, the impact of this transformation seems minimal. This also explains how rare it was for an actual memory bug to occur.

These statistics also justify the small perf impact of this patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133999