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

[Nick Lewycky]

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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20100526/534fd8b1/attachment.html>