Duncan Sands wrote:>> Another option that was discussed in #llvm is to nuke LLVMBuilder and >> rename LLVMFoldingBuilder to LLVMBuilder. If this was the case, I'd >> argue for a flag in the Builder that could retain the old non-folding >> functionality for debugging purposes. >> > > this plan sounds good to me. However it's not clear to me how useful a > debug flag would really be. > >After further discussion in #llvm, and lots of compiling and re-compiling, please find attached the first attempt at the patch with this change. There are two patches attached, one for llvm and one for llvm-gcc42. Please note, there's quite a substantial API change in this patch: LLVMBuilder has been renamed to IRBuilder and has absorbed the constant-folding functionality from LLVMFoldingBuilder. I've updated the tutorial html to refer to the correct includes and classes and tweaked Chapter 4 in particular to explain the constant folding optimizations rather than to explain how to use the LLVMFoldingBuilder to improve the code generation and these changes are included in the patch. This is my first patch so please let me know if there are any problems or anything I need to do differently. Thanks -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: llvm-gcc42.IRBuilder.patch URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20080410/8c36b1a1/attachment.ksh> -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: llvm.IRBuilder.patch URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20080410/8c36b1a1/attachment-0001.ksh>
Dominic Hamon wrote:> Duncan Sands wrote: >>> Another option that was discussed in #llvm is to nuke LLVMBuilder >>> and rename LLVMFoldingBuilder to LLVMBuilder. If this was the case, >>> I'd argue for a flag in the Builder that could retain the old >>> non-folding functionality for debugging purposes. >>> >> >> this plan sounds good to me. However it's not clear to me how useful a >> debug flag would really be. >> >> > > This is my first patch so please let me know if there are any problems > or anything I need to do differently.And there were. updated patches attached. -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: llvm.IRBuilder.patch URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20080410/056c5781/attachment.ksh> -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: llvm-gcc42.IRBuilder.patch URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20080410/056c5781/attachment-0001.ksh>
On Apr 10, 2008, at 7:05 AM, Dominic Hamon wrote:> Dominic Hamon wrote: >> Duncan Sands wrote: >>>> Another option that was discussed in #llvm is to nuke LLVMBuilder >>>> and rename LLVMFoldingBuilder to LLVMBuilder. If this was the >>>> case, I'd argue for a flag in the Builder that could retain the >>>> old non-folding functionality for debugging purposes. >>>> >>> >>> this plan sounds good to me. However it's not clear to me how >>> useful a >>> debug flag would really be. >>> >>> >> >> This is my first patch so please let me know if there are any >> problems or anything I need to do differently.>> > And there were. updated patches attached.Looking good. One big comment: Please attach patches to email instead of including them inline. Some nits:> + Value *CreateMul(Value *LHS, Value *RHS, const char *Name = "") { > + if (Constant *LC = dyn_cast<Constant>(LHS)) > + if (Constant *RC = dyn_cast<Constant>(RHS)) > + return ConstantExpr::getMul(LC, RC); > + return Insert(BinaryOperator::createMul(LHS, RHS, Name)); > + }Please consistently indent by 2, not 4.> > + > + Value *CreateInsertElement(Value *Vec, Value *NewElt, Value *Idx, > + const char *Name = "") {Very picky, but 'const char *Name' should line up with 'Value *Vec'.> Index: gcc/llvm-convert.cpp > ==================================================================> --- gcc/llvm-convert.cpp (revision 49475) > +++ gcc/llvm-convert.cpp (working copy)Please attach llvm-gcc patches as a separate patch file. Otherwise looks great! -Chris
Hi Dominic,
+//===-- llvm/Support/IRBuilder.h - Builder for LLVM Instrs -----*- C++ -*-===//
is this line the right length? It seems shorter than the similar lines below
like
this one:
+//===----------------------------------------------------------------------===//
+ GetElementPtrInst *CreateStructGEP(Value *Ptr, unsigned Idx,
+ const char *Name = "") {
+ llvm::Value *Idxs[] = {
+ ConstantInt::get(llvm::Type::Int32Ty, 0),
+ ConstantInt::get(llvm::Type::Int32Ty, Idx)
+ };
+ return Insert(GetElementPtrInst::Create(Ptr, Idxs, Idxs+2, Name));
+ }
This should constant fold if Ptr is a constant.
+ CallInst *CreateCall(Value *Callee, const char *Name = "") {
+ return Insert(CallInst::Create(Callee, Name));
You should be indenting by two spaces (I think Chris mentioned this).
- DEFINE_SIMPLE_CONVERSION_FUNCTIONS(LLVMFoldingBuilder, LLVMBuilderRef )
+ DEFINE_SIMPLE_CONVERSION_FUNCTIONS(IRBuilder, LLVMBuilderRef )
DEFINE_SIMPLE_CONVERSION_FUNCTIONS(PATypeHolder, LLVMTypeHandleRef )
Please add spaces so that these line up again.
-<p>Well, that was easy :). In practice, we recommend always using
...
+<p>Well, that iss easy :). In practice, we recommend always using
you changed was to iss, I guess you meant "is", though I would prefer
"was".
- LLVMBuilder TmpB(&TheFunction->getEntryBlock(),
+ IRBuilder TmpB(&TheFunction->getEntryBlock(),
TheFunction->getEntryBlock().begin());
Here the arguments no longer line up properly.
- LLVMBuilder TmpB(&TheFunction->getEntryBlock(),
+ IRBuilder TmpB(&TheFunction->getEntryBlock(),
TheFunction->getEntryBlock().begin());
Likewise.
Index: Xcode/LLVM.xcodeproj/project.pbxproj
I don't know what this Xcode stuff is, so I've no idea if it is correct
or whether you should be committing it.
Thanks for doing this!
Best wishes,
Duncan.