[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;
----------------
apostolakis wrote:
> 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
Oh I now realized that your comment was on this check  within isSafeToSinkLoad, and not about skipping cross-BB load sinks in line 760, although the provided statistics should provide more clarity overall.

Regarding this specific check: isSafeToSinkLoad function is only called for cases where both LoadI and SinkPt are in the same BB (see callsite in line 762). Additionally, the iteration here will only work properly for intra-BB searches, so to prevent any misuse it returns conservatively false.

Actually, re-reading this check I could have just called this function and avoid the if-statement in line 760


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133999