[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

[Matthijs Kooijman]

Hi Duncan,

thanks for your comments. I'll reply where appropriate, and fix the rest.

> > +/// 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.
Sounds reasonable, though MarkMaybeAlive is not quite right. It does not
modify any global state (ie, instance variables), just the passed-by-reference
argument (which can be seen as a part of its result, even).

> Plenty of comments don't end in full stops.
What's official policy on this? Any comment should have a full stop? Or just
comments that are complete sentences?

> > +// 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.
Actually, I think this comment is out of date. I'll remove it alltogether :-)

> > +  // Remove any incompatible attributes
> > +  RAttrs &= ~ParamAttr::typeIncompatible(NRetTy);
> If they are incompatible, maybe this means the transform should not
> be performed?
This is what the old code did :-)

Originally, this was mainly for removing attributes concerning the return
type. Ie, if the return type was turned from i32 into void, a sext attribute
can be safely removed. From looking at the typeIncompatible function, this
only concerns attributes for integer and pointer types, so nothing for struct
types. To make this safe for the future, I'll only remove those attributes
when the new type is void, and assert nothing incompatible is present
otherwise.

> > +    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 ?
This breaks for varargs (in which case the CS has more arguments than the
function has paramters).

> > +    for (CallSite::arg_iterator E = CS.arg_end(); I != E; ++I, ++i) {
> I think you forgot to reset i to zero.
Nope, this is again for the varargs case. I'll improve the comments :-)


> > +  /// 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).
How do you mean this? The DAE pass is working with argument numbers
internally, and that is exactly why I need this function (to get the argument
number). In the particular case where I need this function, I need to find the
argument number from a use_iterator, which only knows the operand number.

> > +  /// Returns the operand number of the first argument
> > +  unsigned getArgumentOffset() const {
> How about marking this "inline"?
Shouldn't the compiler notice this by itself?

Gr.

Matthijs
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20080703/2dc60a50/attachment.sig>