[llvm-commits] Dead argument elimination rewrite

[Dan Gohman]

Hi Matthijs,

I've read through this code and it looks good. See my comments below.

On Jun 16, 2008, at 8:37 AM, Matthijs Kooijman wrote:

> Hi all,
>
> I set out to improve deadargelim a bit last week, in particular to  
> allow it to
> look at individual return values instead of just looking at the  
> returned
> struct as a whole.
>
> After some fiddling around with the old code, I decided that the  
> existing
> structure was really unsuited for adaption, actually I suspect that  
> looking at
> return values at all was a later hack-on in the first place.
>
> So, I ended up rewriting the whole thing. Where the old code used two
> iterations (one non-recursive and one recursive) to establish  
> liveness, the
> new code uses only a single iteration, that works non-recursively.

Ok.

>
>
> The pass now looks properly inside returned structs, but only at the  
> first
> level (ie, not inside nested structs). I also have a "flatten return  
> values"
> patch lying around somewhere, which might also become useful for  
> this. Are
> people interested in this?

I don't think it's a high priority, but a "flatten return values" pass
would be nice to have.

>
>
> The pass now also does not look inside functions that use old style  
> multiple
> return instructions, but gracefully ignores those functions (which  
> does have
> small regression, since it also does not remove dead arguments on such
> functions. However, this will be fixed once we stop using the old  
> multiple
> return instructions).

I'm going to be working on having LLVM auto-upgrade old-style multiple
return values to struct returns next. That change will eliminate any
regressions due to not handling them :-).

>
>
> I've also unified the handling of return values and arguments, since
> essentially they should be processed in the same way. This is done  
> by defining
> the RetOrArg struct, which can model either a return value or an  
> argument.
> This also allows for easily putting RetOrArgs into containers.
>
> Liveness of values inspected so far is mainly maintained in the  
> LiveValues
> set. Any value that is clearly live (either because it is used by a  
> RetOrArg
> that we know to be live already or because it is used by some other
> instruction) is put into this set.
>
> Any value that is "MaybeLive", meaning that is used only as an  
> argument or
> return value of which we don't know liveness yet, is tracked in the  
> Uses
> multimap. Or rather, all of its uses are tracked and mapped to the  
> value
> itself, so whenever a RetOrArg is marked live, any other RetToArg it  
> uses can
> also be marked live immediately.
>
> When all instructions are processed, any values not marked live can  
> be assumed
> to be dead and will be removed.
>
> By using a very explicit mapping between what is dependent on what  
> exactly,
> the marking should be efficiently (the old code had to re-establish  
> which
> argument it was supposed to check again, leading to extra looping).
>
> I've attached the patch, but since it changes most of the file, I've  
> also
> attached the new file, which is probably easier to read. The only  
> large part
> remaining from the old code is DeleteDeadVarargs, which is completely
> unmodified.
>
> Comments are welcome!

UseMap is a std::multimap that maps every argument and return value
to every argument or return value that it "uses". That caught my
eye as a potential scalability suspect. Have you done any testing
on large inputs?

The InspectedFunctions member variable looks redundant. Can it be
removed?

In MarkLive:

   UseMap::iterator Begin = Uses.lower_bound(RA);
   UseMap::iterator E = Uses.end();
   UseMap::iterator I;
   for (I = Begin; I != E && I->first == RA; ++I)
     MarkLive(I->second);

This would be a little simpler using Uses.upper_bound(RA).

There are several instances of code like this:

       return Result;
     } else {

Because of the return, it's not necessary to put the following code
in a block under an else. Simplifying these, especially in SurveyUse,
can make the code less nested and easier to read.

Also, watch out for 80 columns :-).

Thanks!

Dan