LLVM LOAD_CONST implementation should skip co_consts

[GoogleCodeExporter]

Currently, the LLVM implementation of the LOAD_CONST opcode indexes into
the code object's co_consts tuple. Since these are *constants*, the
generated machine code should just load the object's address, skipping
co_consts entirely.

Original issue reported on code.google.com by collinw on 29 May 2009 at 3:52

[GoogleCodeExporter]

Original comment by collinw on 29 May 2009 at 4:17

• Added labels: Type-Enhancement

[GoogleCodeExporter]

I am starting work on this ticket.

Original comment by mcquilla...@gmail.com on 30 May 2009 at 1:09

[GoogleCodeExporter]

Original comment by collinw on 30 May 2009 at 2:32

• Changed state: Started

[GoogleCodeExporter]

While just avoiding the lookup through the code object will save a couple memory
accesses, the biggest wins from embedding consts directly would come from 
letting
LLVM's const-propagation passes work over them. For example, if we see the code
"1+a", we currently turn that into an indirect call to
"1"->ob_type->tp_as_number->nb_add(1, a). Exposing the whole definition of "1" 
to
LLVM would let it inline that to the direct call int_add(1, a). We'll have to
clang-compile quite a bit more before it can actually do that, but the 
LOAD_CONST
improvement should be a step toward that.

The general idea for this is to make each Python CONST into an LLVM 
GlobalVariable,
mark it constant, and copy the actual value, transitively, into the initializer.
Unfortunately, if we mark the whole thing constant, we can't do things like 
update
the refcount. Chris Lattner said we should write an AliasAnalysis with
pointsToConstantMemory() defined to take care of this. I haven't double-checked 
that
this actually lets SCCP propagate the initial value places, but Chris is usually
right. :)

Original comment by jyass...@gmail.com on 11 Jun 2009 at 2:44

[GoogleCodeExporter]

I have submitted code for review http://codereview.appspot.com/74058/show

Possible issues: I wasn't quite sure how to handle memory allocation -- I used 
a llvm::OwningArrayPtr for the const_tup 
member, but in reading the code I believe LlvmFunctionBuilder already leaks a 
large amount of memory through non-
deallocated llvm::Values so I didn't do anything with the ConstantExpr's that I 
put into the array.   More sane memory 
management can be used if desired.

re: #4 from my reading of the generated llvm code with this change it looks 
like the following (I'm not including refcount 
instructions because they clutter)

%r1 = co_consts + index //constant is hardcoded in generated IR
push %r1
%r2 = //lookup a
push %r2
%r3 = pop
%r4 = pop
%r5 = call PyNumberAdd(%r3, %r4)

An obvious inefficiency that is not being handled is the explicit use of the 
stack to store then immediately pop values in a 
situation where both operands are available.  However, without some serious 
modifications to the way BinOps work I don't 
see any way to reduce the instructions or put llvm::Constants in place of the 
llvm::Load.

Another issue that comes up attempting to use llvm::Constant values happens in 
regard to say integers, using them in 
context other than addition would need to be handled, which would require some 
juggling between llvm::ConstantInt and 
llvm::Value<PyObject*> in contexts that are not available when the LOAD_CONST 
method is being evaluated.

I have a couple ideas for how to do this, but they all require some major 
modifications to llvm_fbuilder.cc and should 
probably be thought through in a more systematic way than hacking in 
llvm::Constants throughout to handle this one 
case.

Sean

Original comment by mcquilla...@gmail.com on 16 Jun 2009 at 2:55

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

sorry, in the above bytecode I blotched the first line, it should read

%r1 = *(co_consts->ob_item + index)
           // the value (co_consts->ob_item + index) is hardcoded in generated IR

Original comment by mcquilla...@gmail.com on 16 Jun 2009 at 3:06

[GoogleCodeExporter]

Committed Sean's patch in r650. Thanks, Sean!

There's still room to improve on the codegen for LOAD_CONST: the current 
approach 
makes the PyObject*s opaque to LLVM's optimizers, and converting from a 
ConstantExpr::getIntToPtr to a ConstantStruct will expose more data to LLVM. 
I'll open a 
separate issue to track that.

Original comment by collinw on 17 Jun 2009 at 5:32

• Changed state: Fixed

[GoogleCodeExporter]

Opened http://code.google.com/p/unladen-swallow/issues/detail?id=61 to track 
further codegen improvements.

Original comment by collinw on 17 Jun 2009 at 5:39