Issue 42

[Jeffrey Yasskin, 2009-06-16 04:51:49]

Thanks for the patch!
As I mentioned in
http://code.google.com/p/unladen-swallow/issues/detail?id=42#c4, we'll
eventually want to represent these constants with llvm::ConstantStructs, but
this is a good start. The ConstantStructs will only have a big effect after we
inline PyNumber_Add().

http://codereview.appspot.com/74058/diff/1/3
File Python/llvm_fbuilder.cc (right):

http://codereview.appspot.com/74058/diff/1/3#newcode827
Line 827: Value *const_ = this->builder_.CreateLoad(this->consts_tup[index]);
I believe this should be

Value *const_ = ConstantExpr::getIntToPtr(
    ConstantInt::get(
        Type::Int64Ty,
        reinterpret_cast<uintptr_t>(PyTuple_GET_ITEM(co_consts, idx))),
    PyTypeBuilder<PyObject*>::get());

The code you have removes a load from the stack and an add (GEP lowers to an add
to a register, but since that register's virtual and lives through the entire
function, we should assume it will have been spilled), but leaves the load from
the heap (the consts tuple). You really want to remove the load from the heap
too.

ConstantExprs (and more generally, all Constants) in LLVM are uniqued but then
never destroyed, so you get memory use proportional to the total number and
complexity of constants you ever use. Instructions are owned by the function
they appear in and deleted when they're no longer used.

http://codereview.appspot.com/74058/diff/1/3#newcode2298
Line 2298: //create Value<PyObject**> array of &(co_consts->ob_item[i]). 
PyObject**
Please put a space after all "//"s and capitalize your comments.

http://codereview.appspot.com/74058/diff/1/3#newcode2301
Line 2301: for(Py_ssize_t idx = 0; idx < sz; idx += 1)
Control structures get a space between the keyword and the opening '('. The
brace also goes on the same line as the closing ')', not on a separate line. We
only use the separate line convention for functions.

http://codereview.appspot.com/74058/diff/1/2
File Python/llvm_fbuilder.h (right):

http://codereview.appspot.com/74058/diff/1/2#newcode422
Line 422: llvm::OwningArrayPtr<llvm::Value *> consts_tup;
Member variables end in a '_'.

[Collin Winter, 2009-06-17 14:48:11]

I've cleaned up Sean's patch and put it in http://codereview.appspot.com/75077,
which contains performance results and before/after IR comparisons. If you can
look it over, Jeff, I'll go ahead and commit it.

[Collin Winter, 2009-06-17 16:24:22]

On 2009/06/17 14:48:11, Collin Winter wrote:
> I've cleaned up Sean's patch and put it in
http://codereview.appspot.com/75077,
> which contains performance results and before/after IR comparisons. If you can
> look it over, Jeff, I'll go ahead and commit it.

Committed as r650. Thanks for the patch, Sean!