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

[Nick Lewycky]

On 02/19/2011 08:43 PM, Andrew Clinton wrote:
> Thank you for the review - attached is the revision.

Excellent!

> If you could commit this to the baseline that would be appreciated!  I
> have a few more patches that I'm planning to submit in the next week.

I cleaned it up a little and committed it as r126077. Thanks for the patch!

Nick

> Andrew
>
> On Sat, Feb 19, 2011 at 8:44 PM, Nick Lewycky<nicholas at mxc.ca>  wrote:
>> 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
>>
>>