[PATCH] D118143: [GVN] Support load of pointer-select to value-select conversion.

[Philip Reames via Phabricator via llvm-commits]

reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

This wasn't quite what I'd expected, but this approach appears to be workable, if not fully generic.

The fully generic result I was expecting was for you to generalize the available block mechanism into a mapping per address of availability per block plus a side structure to remember the address translation.  (We implicitly already have the later for phi nodes, but we'd have to track it explicitly for selects.)

The general mechanism is a much deeper change, so I understand if you don't want to do it now.



================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:967
+        if (LI && LI->getParent() == Sel->getParent() &&
+            gvn.getDominatorTree().dominates(LI, Sel))
+          return LI;
----------------
I wasn't sure GVN already relied on comesBefore caching and thus was initial concerned this might be a performance bottleneck, but it looks like there's a bunch of cases we already assume block order is up to date, so this should be a non-issue.  


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:978
+           "the select");
+    Value *NewSel = SelectInst::Create(Sel->getCondition(), L1, L2, "", Sel);
+    Res = NewSel;
----------------
Remove the extra def?


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1214
+
+  auto FindDominatingLoad = [Sel, &DT](Value *Ptr) -> LoadInst * {
+    for (Value *U : Ptr->users()) {
----------------
Repeated code, pull out a static function.  


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1228
+
+  return AvailableValueInBlock::getSelect(Sel->getParent(),
+                                          cast<SelectInst>(Address));
----------------
Unless I'm really missing something obvious, you've got a serious omission here.  

The memory dependence walk already done tells us there's no clobber between the select and the using loads.  I do not see anything in the added code which checks for a clobber between select and the loads above it.  

L1 = load A1
L2 = load A2
clobber
S = select A1, A2
L3 = load S


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1257
+      if (auto R =
+              tryToConvertLoadOfPtrSelect(DepBB, Address, getDominatorTree())) {
+        ValuesPerBlock.push_back(*R);
----------------
You appear to only be handling the case where the select and it's loads are in a different block from the load being removed.  Please add the corresponding case for block local code as well.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118143