[llvm-commits] [llvm] r52677 - in /llvm/trunk:?lib/Transforms/IPO/DeadArgumentElimination.cpp?test/Tra nsforms/DeadArgElim/2008-06-23-DeadAfterLive.ll?test/Transforms/Dead ArgElim/deadretval2.ll?test/Transforms/DeadArgElim/multdeadretval.ll

[Duncan Sands]

Hi,

> +    /// Struct that represent either a (part of a) return value or a function

-> Struct that represents (part of) either a return value or a function

> +        return std::string((IsArg ? "Argument #" : "Return value #")) + IdxStr + " of function " + F->getName();

This line is too long.

> +    typedef std::multimap<RetOrArg, RetOrArg> UseMap;
> +    /// This map maps a return value or argument to all return values or

This map maps -> This maps

> +/// IsMaybeAlive - This checks Use for liveness. If Use is live, returns Live,
> +/// else returns MaybeLive. Also, adds Use to MaybeLiveUses in the latter case.

I don't much like predicates that modify state.  Maybe you could change the name
to MarkMaybeAlive or something like that, so conceptually the return value becomes
information about the operation performed.

> +  // We're live if our use is already marked as live

Plenty of comments don't end in full stops.

> +        assert(CS.getArgument(ArgNo) == CS.getInstruction()->getOperand(U.getOperandNo()) && "Argument is not where we expected it");

This line is too long.

> +  // These vectors maps each return value to the uses that make it MaybeLive, so

These vectors maps -> These vectors map

> +  // Intialized to a list of RetCount empty lists

Intialized -> Initialized

> +// that are not in LiveValues. This function is a noop for any Function created
> +// by this function before, or any function that was not inspected for liveness.

Too many uses of function!  How about:
This is a noop for any Function created here before, or any Function that was not inspected for liveness.

> +  // Explicitely track if anything changed, for debugging

Explicitely -> Explicitly

> +  // Remove any incompatible attributes
> +  RAttrs &= ~ParamAttr::typeIncompatible(NRetTy);

If they are incompatible, maybe this means the transform should not
be performed?

> +  // Not that we apply this hack for a vararg fuction that does not have any

Not that -> Note that

> +    CallSite::arg_iterator I = CS.arg_begin();
> +    unsigned i = 0;
> +    // Loop over those operands, corresponding to the normal arguments to the
> +    // original function, and add those that are still alive.
> +    for (unsigned e = FTy->getNumParams(); i != e; ++I, ++i)

How about controlling the loop using I not i, i.e. define E = CS.arg_end()
and test I != E ?

> +    for (CallSite::arg_iterator E = CS.arg_end(); I != E; ++I, ++i) {

I think you forgot to reset i to zero.

> +          // This does generate messy code, but we'll let it to instcombine to

We'll let it to -> We'll leave it to

> +  /// Given an operand number, returns the argument that corresponds to it.
> +  /// OperandNo must be a valid operand number that actually corresponds to an
> +  /// argument.

How about working with argument numbers rather than operand numbers?  Then
this wouldn't be needed (and everything would start at 0).

> +  /// Returns the operand number of the first argument
> +  unsigned getArgumentOffset() const {

How about marking this "inline"?

Ciao,

Duncan.