[llvm-commits] [PATCH] [ARM JIT] To fix "llvm_unreachable("Unable to handle this constantpool entry!")"

[Zonr Chang]

Let me have a try.

The attached patch combines arm-jit-ConstantVector-ConstantArray.patch
and arm-jit-LEApcrel.patch provided by Shih-Wei with typo/coding fixing
mentioned by Nick. The test cases used in PR7226 and PR7227 are the same.
And only with this combined patch, test case can be successfully JIT'ed and
executed. I simplified the test case as below:

==== BEGIN TEST CASE SNIPPET ====
int gTextureSwap;
float scale[1];

int main(int argc, char** argv) {
  if (gTextureSwap != 0)
      scale[0] = .25f;
  else
      scale[0] = 4.f;

  return 55;
}
==== END TEST CASE SNIPPET ====

The assembly showed after JIT is (by disassemble the result binary):

JIT: Disassembled code: main
    e3060ff8 movw r0, #28664
    e34100ca movt r0, #4298
    e3a02000 mov r2, #0
    e5900000 ldr r0, [r0]
    e3500000 cmp r0, #0
    e3a00000 mov r0, #0
    3a00001 moveq r0, #1
    e3500000 cmp r0, #0
    13a02004 movne r2, #4
    e28f0018 add r0, pc, #24 ; require pseudo instruction LEApcrel
    e0800002 add r0, r0, r2
    ed900a00 vldr.32 s0, [r0]
    e3060ff4 movw r0, #28660
    e34100ca movt r0, #4298
    ed800a00 vstr.32 s0, [r0]
    e3a00037 mov r0, #55
    e12fff1e bx lr
    3e800000 cdplo p0, #8, cr0, cr0, cr0, #0 ; 0x3e800000 = 0.25
    40800000 addmi r0, r0, r0 ; 0x40800000 = 4.0

In this case, as mentioned in Liao's bug report, 0.25f and 4.f in the code
above will become ConstantArray of 2 elements. And I'm not sure whether
there's any needed implementation for ConstantVector in
emitConstantToMemory(...). To avoid to deal with the alignment and padding
problems arising from materializing vector type, I just leave it out (I try
to find a test case that will need vector constant materialization but no
any result up to now).

However, here comes another question. Does anyone know what kind of
"Constant" can be in emitConstantToMemory(...)?


Zonr


On Thu, May 27, 2010 at 11:41 AM, Nick Lewycky <nlewycky at google.com> wrote:

> Hi Shih-wei, I have a few comments:
>
> +void ARMCodeEmitter::emitConstantToMemory(unsigned CPI, const Constant* C)
> {
>
> The rest of the code uses "const Constant *C". Please place the space
> before the star.
>
> In X86CodeEmitter, the
>
> +  } else if (const ConstantVector *CV = dyn_cast<ConstantVector>(C)) {
> +    for (unsigned i=0;i<CV->getNumOperands();i++)
> +      emitConstantToMemory(CPI, CV->getOperand(i));
> +  } else if (const ConstantArray *CA = dyn_cast<ConstantArray>(C)) {
> +    for (unsigned i=0;i<CA->getNumOperands();i++)
> +      emitConstantToMemory(CPI, CA->getOperand(i));
> +  } else {
> +    llvm_unreachable("Unable to handle this constantpool entry!");
> +  }
>
> A few things. Firstly, please use proper whitespace, make your code look
> like the code around you. This means "for (unsigned i = 0; i <
> CA->getNumOperands(); i++)". Secondly, always hoist out the end computation
> of a loop, which would make it "for (unsigned i = 0, e =
> CV->getNumOperands(); i != e; i++)".
>
> Now, you've added a lot of cases and I'm wondering whether it's time to
> switch away from the else-if nesting. As it is, the dyn_cast<> macros have
> to be tried in the order they're written in the code. Using a switch only
> evaluates the concrete type once, and makes the code:
>
> switch (C->getValueID()) {
>   case GlobalVariableVal:  // fall-through
>   case GlobalAliasVal:  // fall-through
>   case FunctionVal:  {
>     GlobalValue *GV = cast<GlobalValue>(C);
>     emitGlobalAddress ...
>     break;
>   }
>   case ConstantIntVal: {
>     ConstantInt *CI = cast<ConstantInt>(C);
>     ...
>     break;
>   }
>   ...
> }
>
> I also noticed that you've added potentially unbounded recursion (well,
> it's bounded by the depth of nesting of array types), but that doesn't look
> easy to fix, and it's probably not going to be a problem anyways (ie., we'd
> run out of ArrayType objects first).
>
> Now finally, with all that said, I'm still not sure about this patch. Is
> the layout of a vector in memory the same as an array in memory? You're
> treating them the same with no padding or alignment issues considered, just
> stamping the elements into memory. Someone else should look at that before
> this patch lands.
>
> Nick
>
> On 26 May 2010 02:27, Shih-wei Liao <sliao at google.com> wrote:
>
>> See http://llvm.org/bugs/show_bug.cgi?id=7226: ARM JIT couldn't emit
>> constant pool entry for "ConstantVector" and "ConstantArray".
>>
>> The patch passes both "make check" and "make check-lit". Basically,
>> the patch added the processing of ConstantVector and ConstantArray.
>> The problem is gone after the patch.
>> --
>> Thanks,
>>   Shih-wei
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20100527/5fd0122a/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: arm-jit-LEApcrel2.patch
Type: application/octet-stream
Size: 6366 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20100527/5fd0122a/attachment.obj>