Keyboard Shortcuts

File
u :up to issue
m :publish + mail comments
M :edit review message
j / k :jump to file after / before current file
J / K :jump to next file with a comment after / before current file
Side-by-side diff
i :toggle intra-line diffs
e :expand all comments
c :collapse all comments
s :toggle showing all comments
n / p :next / previous diff chunk or comment
N / P :next / previous comment
<Up> / <Down> :next / previous line
<Enter> :respond to / edit current comment
d :mark current comment as done
Issue
u :up to list of issues
m :publish + mail comments
j / k :jump to patch after / before current patch
o / <Enter> :open current patch in side-by-side view
i :open current patch in unified diff view
Issue List
j / k :jump to issue after / before current issue
o / <Enter> :open current issue
# : close issue
Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :save comment
<Esc> :cancel edit
Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(36)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 74058: Issue 42

Can't Edit
Can't Publish+Mail
Start Review
Created:
16 years, 7 months ago by mcquillan.sean
Modified:
16 years, 7 months ago
CC:
unladen-swallow_googlegroups.com
Base URL:
http://unladen-swallow.googlecode.com/svn/trunk/
Visibility:
Public.

Patch Set 1 #

Total comments: 4
Created: 16 years, 7 months ago
Download [raw] [tar.bz2]
Unified diffs Side-by-side diffs Delta from patch set Stats (+177 lines, -11 lines) Patch
A Lib/test/test_llvm_load_const.py View 1 chunk +132 lines, -0 lines 0 comments Download
M Python/llvm_fbuilder.h View 3 chunks +10 lines, -0 lines 1 comment Download
M Python/llvm_fbuilder.cc View 4 chunks +35 lines, -11 lines 3 comments Download
Total messages: 4
|
mcquillan.sean
16 years, 7 months ago (2009年06月15日 23:45:17 UTC) #1
Sign in to reply to this message.
Jeffrey Yasskin
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 ...
16 years, 7 months ago (2009年06月16日 04:51:49 UTC) #2
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 '_'.
Sign in to reply to this message.
Collin Winter
I've cleaned up Sean's patch and put it in http://codereview.appspot.com/75077, which contains performance results and ...
16 years, 7 months ago (2009年06月17日 14:48:11 UTC) #3
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.
Sign in to reply to this message.
Collin Winter
On 2009年06月17日 14:48:11, Collin Winter wrote: > I've cleaned up Sean's patch and put it ...
16 years, 7 months ago (2009年06月17日 16:24:22 UTC) #4
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!
Sign in to reply to this message.
|
Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b

AltStyle によって変換されたページ (->オリジナル) /