[PATCH] D16774: AMDGPU: Stop pointlessly looking at users after a bad one is found

[David Blaikie via llvm-commits]

On Mon, Feb 1, 2016 at 1:13 PM, Matt Arsenault <Matthew.Arsenault at amd.com>
wrote:

> On 02/01/2016 12:48 PM, David Blaikie wrote:
>
>
>>>
>> Why not just drop the Success variable entirely and return from here:
>>
>> if (!collectUsesWithPtrTypes(...))
>>   return false
>>
>> Return here is what I tried first, but this breaks some cases due to the
>> !isPointerTy check above. Call users without pointer types would then fall
>> through and incorrectly be changed.
>>
>
> Sorry, couldn't really follow... the original code would return false if
> collectUsesWithPtrTypes ever returns false, right? Were there side effects
> that were desired after that point?
>
>
> collectUsesWithPtrTypes should only be called with a pointer type value.
> Assuming there is an intrinsic call that uses a pointer and returns an
> integer for example, the User->getType()->isPointerTy() check will cause
> the call user to not be added to the worklist, because we do not want to
> have non-pointers in the worklist. I should probably just add another
> pointer check under the call handling so it stops falling through to here.
>

Sorry, think I'm talking past you/not making sense. I haven't
looked/understood the actual purpose of this loop - I'm just looking at it
mechanically.

The original code was:

  for (element) {
    if (!work(element))
      continue;
    Success &= func(element);
  }
  return Success;

So, previously work was called for every element, and if func(element) ever
returned false for a relevent (where work returns true) , the function
returned false (otherwise true)

The new code you have is:

  for (element) {
    if (!Success)
      return false;
    if (!work(element))
      continue;
    Success = func(element);
  }
  return true;

This has the same behavior as the original code, except for the final
iteration - if func(element) returns false on the last iteration through
the loop, the function will now return true instead of false. Is that
intended? (if so, a test would be good) or is it just an impossible state?

Why wait for the next run around the loop to check Success and return?
Rather than just returning immediately:

  for (element) {
    if (!work(element))
      continue;
    if (!func(element))
      return false;
  }
  return true;

This has the same behavior as the original code (doesn't have the change in
behavior for the last iteration through the loop). If that last iteration
isn't important (the difference in behavior isn't intentional or important
(because it never comes up that way, etc)) this seems simpler - the only
difference then is that one returns immediately, the other increments the
for loop iterator once more, then returns - which doesn't seem important?

- Dave
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160201/f60349e9/attachment.html>