[llvm-commits] [llvm] r58405 - r58409

[Evan Cheng]

On Oct 30, 2008, at 1:40 PM, Jim Grosbach wrote:

>
> On Oct 30, 2008, at 10:48 AM, Evan Cheng wrote:
>>
>>>>
>>>> +      MCE.emitWordLE(0);
>>>> +    } else {
>>>> +      abort(); // FIXME: Is this right?
>>>> +      const ConstantInt *CI = dyn_cast<ConstantInt>(CV);
>>>> +      uint32_t Val = *(uint32_t*)CI->getValue().getRawData();
>>>> +      MCE.emitWordLE(Val);
>>>> +    }
>>>
>>>
>>> This breaks simple cases like the following:
>>> #include <stdio.h>
>>>
>>> int main(int argc, char *argv[])
>>> {
>>> unsigned a;
>>>
>>> a = 0xaaaaaaaa;
>>> }
>>>
>>> These were working before and hit this abort now.
>>
>> Right. It's intended. I want a better solution for all the cases.  
>> I'll
>> deal with this shortly.
>
> A stronger solution later would be great. I don't see the need for the
> abort(), though. That breaks working code. Just the FIXME, perhaps
> with an explanatory note, should suffice.

Having an abort is useful because it allows us to use bugpoint to  
reduce test cases.

>
>
>>>
>>> I have mixed feeling about this. On the one hand, it makes sense to
>>> have relocation related bits (pun intended) be part of the  
>>> relocation
>>> class. On the other hand, this implies that the target will be  
>>> mixing
>>> and matching whether a constant pool entry will be resolved by the
>>> target or by the generic code on a per-constant basis, which is not
>>> true. The decision to put the constants elsewhere is made on an all-
>>> or-
>>> nothing basis, and it seems that this should mirror that decision.  
>>> On
>>
>> That's true for ARM. But it may not always be true. The main reason
>> for the change is the old scheme cannot deal with machine  
>> constantpool
>> entries. Also, this is a clearer separation. The CONSTPOOL_ENTRY is  
>> an
>> instruction, so the target code emitter should populate it.
>
> Machine constantpool entries are an orthogonal issue.
>
>>> Perhaps the implication is that the non-target specific JIT  
>>> shouldn't
>>> be doing the constant pool layout, and should be delegating that,  
>>> and
>>> the associated relocation address resolution, to the target always?
>>>
>
> This is the root of it, I believe. If the constant pool layout is
> always part of the target, a lot of the strangeness of this stuff goes
> away. If we want a simple default implementation as part of the parent
> class, that's perfectly reasonable, but it seems it doesn't belong in
> the ExecutionEngine/JIT target independent portions.

Perhaps the right solution is to move this out to TargetJITInfo. That  
allows us to provide a generic implementation.

Evan

>
>
> -Jim
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits