[PATCH] [GVN] Eliminate redundant loads whose addresses are dependent on the result of a select instruction.

[Philip Reames]

A few comments inline.  I didn't spot any obvious problems with the code, but it feels like you're trying to solve a problem in one step that should probably be sub-divided into several.  I suggested one possible division, but there might be others that make more sense.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:1855
@@ +1854,3 @@
+                           Value *SelectOp, DominatorTree *DT) {
+  for (Value::user_iterator UI = SelectOp->user_begin(),
+                            UE = SelectOp->user_end();
----------------
Another way you might be able to phrase this would be to ask GVN if it had a value which would be CSEd with a newly created GEP at the current instruction.  

================
Comment at: lib/Transforms/Scalar/GVN.cpp:1868
@@ +1867,3 @@
+
+    // Assume a single use of the GEP.
+    if (!SelectGEP->hasOneUse())
----------------
This restriction feels like a hack.

================
Comment at: lib/Transforms/Scalar/GVN.cpp:1903
@@ +1902,3 @@
+//
+bool GVN::loadGEPSelect(LoadInst *LI, MemDepResult Dep) {
+  Value *Op = LI->getOperand(0);
----------------
Ignoring your code entirely for a moment....

Your starting IR fragment seems really odd to me.  I'd have expected this to be canonicalized as:
// a_addr = gep ptr_op idx idx_a
// b_addr = gep ptr_op idx idx_b
// a = (ld|st a_addr
// b = (ld|st b_addr
// c = select (cond, a_addr, b_addr)
// c = load c_addr

This form would make your transform far more obvious once we actually got to PRE.  

Thoughts?

================
Comment at: lib/Transforms/Scalar/GVN.cpp:1915
@@ +1914,3 @@
+    // Look through casts.
+    if (CastInst *Cast = dyn_cast<CastInst>(Op))
+      Op = Cast->getOperand(0);
----------------
Why only one level?

http://reviews.llvm.org/D8120

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/