llvm dev - Apr 2008 - [LLVMdev] LLVMBuilder vs LLVMFoldingBuilder

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(&amp;TheFunction-&gt;getEntryBlock(),
+  IRBuilder TmpB(&amp;TheFunction-&gt;getEntryBlock(),
                    TheFunction-&gt;getEntryBlock().begin());

Here the arguments no longer line up properly.

-  LLVMBuilder TmpB(&amp;TheFunction-&gt;getEntryBlock(),
+  IRBuilder TmpB(&amp;TheFunction-&gt;getEntryBlock(),
                    TheFunction-&gt;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.

Duncan Sands wrote:
> +  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.
>
>   
While looking at this, I noticed that GetElementPtrInst::Create and 
ConstantExpr::getGetElementPtr have a different interface when taking a 
list of indices. The first takes the start and end pointers to the list, 
while the second takes the first pointer and the length of the list. I 
feel like these interfaces should be consistent. Maybe my next
patch.
> 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.
>   
I've added some missing files, removed LLVMBuilder.h and added 
IRBuilder.h and added the BrainF example (as I had to make changes to it 
for the patch).

The only changes that I'm not 100% happy with are the 
ShadowStackCollector changes. The code will currently assert if the 
BasePtr passed in to the ShadowStackCollector is a constant as the GEP 
will be a constant. If we want to make the ShadowStackCollector support 
constant folding, more changes are required. None of the tests in the 
DejaGNU test suite or the full llvm test-suite have asserted, so I have 
to believe this is safe. If someone with a greater understanding of how 
the ShadowStackCollector works could give input on this, I'd be grateful.

Please find _attached_ the update patches with all requested changes.

Thanks
Dominic

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: llvm-gcc42.IRBuilder.patch
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20080412/8317f8be/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/20080412/8317f8be/attachment-0001.ksh>

On 2008-04-12, at 03:10, Dominic Hamon wrote:
> The only changes that I'm not 100% happy with are the  
> ShadowStackCollector changes. The code will currently assert if the  
> BasePtr passed in to the ShadowStackCollector is a constant as the  
> GEP will be a constant.

These utilities are only ever used with AllocaInst*'s; your assertion  
and dyn_cast are respectively dead and unnecessary. Please try  
dropping the dyn_cast and the dead assertion in favor of just relaxing  
the return type:

-    static GetElementPtrInst *CreateGEP(LLVMBuilder &B, Value *BasePtr,
+    static Value *CreateGEP(IRBuilder &B, Value *BasePtr,

— Gordon

> Please find _attached_ the update patches with all requested changes.
Applied - thanks!

Ciao,

Duncan.

Hi Dominic,
> While looking at this, I noticed that GetElementPtrInst::Create and 
> ConstantExpr::getGetElementPtr have a different interface when taking a 
> list of indices. The first takes the start and end pointers to the list, 
> while the second takes the first pointer and the length of the list. I 
> feel like these interfaces should be consistent. Maybe my next patch.
please change ConstantExpr::getGetElementPtr to be like
GetElementPtrInst::Create.

Thanks!

Duncan.