[llvm-commits] Patch to delete PHI nodes with multiple uses

[Nick Lewycky]

Andrew Clinton wrote:
> This is a simple patch to improve RecursivelyDeleteDeadPHINode so that
> it will delete PHIs that have multiple identical uses (self
> references).

Cool! I have some review comments:

+// Check whether the uses of a value are all the same.  This is similar to
+// Instruction::hasOneUse() except this will also return true when 
there are
+// multiple uses that all refer to the same value.
+static bool
+AreAllUsesEqual(Instruction *J)
+{

The previous three lines all fit on one. Also, why "J"? "I" isn't used 
in this function. Also, since this is a new function, please start the 
name with an initial lowercase letter 
(http://llvm.org/docs/CodingStandards.html#ll_naming is newer than most 
of the code, in case you're wondering).

+  Value::use_iterator UI = J->use_begin();
+  Value::use_iterator UE = J->use_end();
+  if (UI == UE)
+    return false;
+
+  for (++UI; UI != UE; ++UI)
+  {
+    if (*UI != *J->use_begin())

Could you hoist the computation of *J->use_begin() out of the loop?

+      return false;
+  }
+  return true;
+}

> The DeletePHIs.cpp is a pass to test it, along with delete-phis.ll.
> I'm not sure whether this test needs to be added to the baseline or if
> so, where to put it.  Run the test with:

The delete-phis.ll test is convincing enough, I don't think this 
warrants keeping an entire pass -- but if you do want to run this method 
over some code, I suggest writing a unit test for it under unittests/. 
You can produce a Function then call RecursivelyDeleteDeadPHINode on an 
instruction and check the result.

Please post an updated patch. Do you want me to commit this for you?

Nick

> opt -load ../../../Debug+Asserts/lib/LLVMDeletePHIs.so<
> delete-phis.ll -delete-phis -S
>
> Andrew
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits