[llvm-commits] [llvm] r58405 - r58409

[Jim Grosbach]

On Oct 29, 2008, at 4:54 PM, Evan Cheng wrote:
>
> void JITEmitter::emitConstantPool(MachineConstantPool *MCP) {
> -  if (TheJIT->getJITInfo().hasCustomConstantPool()) {
> -    DOUT << "JIT: Target has custom constant pool handling.  
> Omitting standard "
> -            "constant pool\n";
> -    return;
> -  }

If this is removed, the JIT will still emit the generic constant pool  
at the start of the function. The ARM backend doesn't use that pool  
(all the constants are put into the constant islands), so the generic  
pool is a waste of space and completely unreferenced. There needs to  
be a way for the target to tell the generic bits that the target is  
handling all of the constant pool entries.

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

> --- llvm/trunk/include/llvm/CodeGen/MachineRelocation.h (original)
> +++ llvm/trunk/include/llvm/CodeGen/MachineRelocation.h Wed Oct 29  
> 18:53:42 2008
> @@ -63,10 +63,11 @@
>     unsigned GOTIndex;      // Index in the GOT of this symbol/global
>   } Target;
>
> -  unsigned TargetReloType : 6; // The target relocation ID.
> -  AddressType AddrType    : 4; // The field of Target to use.
> -  bool NeedStub           : 1; // True if this relocation requires  
> a stub.
> +  unsigned TargetReloType : 6; // The target relocation ID
> +  AddressType AddrType    : 4; // The field of Target to use
> +  bool NeedStub           : 1; // True if this relocation requires  
> a stub
>   bool GOTRelative        : 1; // Should this relocation be relative  
> to the GOT?
> +  bool TargetResolve      : 1; // True if target should resolve the  
> address

I thought we were avoiding adding entries to the MachineRelocation  
class?

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  
the gripping hand, I like that this allows for target-specific stuff  
that's not just the constant pool entries.

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?

As a sidenote, shouldn't these all have been a single commit rather  
than one per file? The changes are related and require one another for  
a successful build. Having related changes be checked in as an atomic  
commit is very useful when doing forensics later.


-Jim