[patch] FastISel omitting unused parameters hurts debug info

David Blaikie dblaikie at gmail.com
Fri Jun 21 15:59:01 PDT 2013


On Mon, Jun 10, 2013 at 2:54 PM, Evan Cheng <evan.cheng at apple.com> wrote:
>
>    // Enter non-dead arguments into ValueMap for uses in non-entry BBs.
>    for (Function::const_arg_iterator I = FuncInfo.Fn->arg_begin(),
>           E = FuncInfo.Fn->arg_end(); I != E; ++I) {
> -    if (!I->use_empty()) {
> -      DenseMap<const Value *, unsigned>::iterator VI = LocalValueMap.find(I);
> -      assert(VI != LocalValueMap.end() && "Missed an argument?");
> +    DenseMap<const Value *, unsigned>::iterator VI = LocalValueMap.find(I);
> +    if (VI != LocalValueMap.end())
>        FuncInfo.ValueMap[I] = VI->second;
> -    }
>
> I think you need to update the comment. At least the part about "non-dead".

Updated & committed as r184604.

> I'm neutral about this patch. I don't believe it will have material impact on compile time but I do wonder if there is another way to fix this.

Perhaps - for stack parameters we do have an extra table of those we
use when optimizations kick in, but to keep track of clobbers,
especially with in-register params, it seems necessary/appropriate to
use the rest of the usual isel infrastructure.

Thanks,
- David

>
> Evan
>
>
> On Jun 6, 2013, at 7:52 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>> Hi Dan,
>>
>> Here's the patch we talked about this afternoon - with the equivalent
>> change made to the ARM side of fast ISel to match the X86 change I'd
>> made. Turns out with the ARM change something does fail -
>> CodeGen/ARM/fast-isel-call.ll. So it seems this change isn't neutral
>> to codegen. (though I'm not sure how much we need to care deeply about
>> the performance of unused parameters?) I assume the source of the
>> change is the FIXME immediately proceeding the change to *FastISel.cpp
>> - so perhaps this is an acceptable "we'll fix the underlying issue &
>> this and some other things will get better".
>>
>> I was wondering if you had any ideas on how this should be addressed
>> (is this acceptable? in which case I should just update the test case
>> (though I don't fully understand it yet). I've attached the two .s
>> files, with my change (a.s) and without my change (b.s) (around
>> r183465) along with the patch itself.
>>
>> - David
>> <a.s><b.s><missing_unused_parameter.diff>_______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>



More information about the llvm-commits mailing list