<div><div>Hi Shih-wei, I have a few comments:</div><div><br></div><div>+void ARMCodeEmitter::emitConstantToMemory(unsigned CPI, const Constant* C) {</div></div><div><br></div><div>The rest of the code uses "const Constant *C". Please place the space before the star.</div>

<div><br></div><div>In X86CodeEmitter, the </div><div><br></div><div>+  } else if (const ConstantVector *CV = dyn_cast<ConstantVector>(C)) {</div><div>+    for (unsigned i=0;i<CV->getNumOperands();i++)</div><div>

+      emitConstantToMemory(CPI, CV->getOperand(i));</div><div>+  } else if (const ConstantArray *CA = dyn_cast<ConstantArray>(C)) {</div><div>+    for (unsigned i=0;i<CA->getNumOperands();i++)</div><div>+      emitConstantToMemory(CPI, CA->getOperand(i));</div>

<div>+  } else {</div><div>+    llvm_unreachable("Unable to handle this constantpool entry!");</div><div>+  }</div><div><br></div><div>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++)".</div>

<div><br></div><div>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:</div>

<div><br></div><div>switch (C->getValueID()) {</div><div>  case GlobalVariableVal:  // fall-through</div><div>  case GlobalAliasVal:  // fall-through</div><div>  case FunctionVal:  {</div><div>    GlobalValue *GV = cast<GlobalValue>(C);</div>

<div>    emitGlobalAddress ...</div><div>    break;</div><div>  }</div><div>  case ConstantIntVal: {</div><div>    ConstantInt *CI = cast<ConstantInt>(C);</div><div>    ...</div><div>    break;</div><div>  }</div><div>

  ...</div><div>}</div><div><br></div><div>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).</div>

<div><br></div><div>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.</div>

<div><br></div><div>Nick</div><div><br></div><div class="gmail_quote">On 26 May 2010 02:27, Shih-wei Liao <span dir="ltr"><<a href="mailto:sliao@google.com">sliao@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">

See <a href="http://llvm.org/bugs/show_bug.cgi?id=7226" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=7226</a>: ARM JIT couldn't emit<br>
constant pool entry for "ConstantVector" and "ConstantArray".<br>
<br>
The patch passes both "make check" and "make check-lit". Basically,<br>
the patch added the processing of ConstantVector and ConstantArray.<br>
The problem is gone after the patch.<br>
--<br>
Thanks,<br>
<font color="#888888">  Shih-wei<br>
</font><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br>