[llvm-commits] [llvm] r59266 - in /llvm/trunk: include/llvm/Target/TargetLowering.h lib/Target/X86/X86ISelLowering.cpp lib/Target/X86/X86RegisterInfo.td

[Evan Cheng]

I have mixed feelings about this solution. That said, why does isel  
need to pre-assign registers if the new register class ensures the  
right allocation is done?

Evan

On Nov 16, 2008, at 4:48 PM, Dale Johannesen <dalej at apple.com> wrote:

>
> On Nov 15, 2008, at 7:32 PM, Chris Lattner wrote:
>>>> Does llc slow down
>>>> at "-O0" by adding another register class?
>>>
>>>> Finally, how does this fix the bug?  Is isel still doing the
>>>> 'register
>>>> allocation' of the 'A' constraint, or is it turning it into vregs
>>>> from
>>>> the GRAD reg class?  I thought the bug was that the isel regalloc
>>>> was
>>>> assigning the tied register to EAX and then it just wasn't  
>>>> available
>>>> when handling the A constraint.
>>>
>>>
>>> ISel is allocating the registers for both arguments before and after
>>> the patch.  Since 'A' and the tied register were both previously
>>> treated as C_RegisterClass, they were allocated in argument order,
>>> which led to the problem you say.  Now that 'A' is marked as using a
>>> specific hard register, it gets allocated before the tied register,
>>> which then knows it cannot use EAX and EDX.
>>
>> Ok.  My problem with this is that adding register classes to the
>> target *just* to support inline asm seems bad.  Inline is an evil
>> menace to the code generator, and IMO it should have as minimal  
>> impact
>> on it as possible.
>
> I don't like inline asm either.  I don't know a compiler person who
> does.  That's irrelevant; we need to support it, and supporting it
> badly is not going to make it go away.  I'm strongly of the opinion
> this is the cleanest and most elegant solution for this problem.
>
>> I'd be much happier with changing the SDISel code to do an extra pass
>> over the input/output list of the asm to look for these and handle
>> them early if that would solve the problem.  This way, only inline  
>> asm
>> pays the compile-time cost, and the targets don't have to sprout new
>> reg classes to match the GCC constraint letters.  Do you think this  
>> is
>> a reasonable approach?
>
> It is already doing a pass to handle fixed-register constraint
> letters.  In other cases these are marked specially by the front end,
> as "{eax}".  I did consider extending that notation to allow "{eax
> +edx}", but the register class solution is much simpler and cleaner
> than changing all front ends, plural.  Further, it handles a target-
> dependent problem in the target code, where it belongs.
>
> I really don't understand your objection to adding register classes.
> This is free aside from an unnoticeable amount of compile time, and a
> good general mechanism.  Actually I think it would be preferable to
> handle the other fixed-register constraints, like "d", with register
> classes and rip all that conversion crap out of the front end (or
> ends, if it's gotten into clang already).  Not that I'm planning to do
> that.
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits