[llvm-commits] [llvm] r58405 - r58409

[Jim Grosbach]

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.

>>
>> 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.

-Jim