llvm dev - Oct 2009 - [LLVMdev] Some additions to the C bindings

My front-end is sync'd with the trunk now, and working well, but it
required some additional functions exposed in the C bindings.  I
hereby submit them for review and approval for inclusion in the trunk.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cbindings.patch
Type: application/octet-stream
Size: 7269 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20091006/274b4675/attachment.obj>

On Tue, Oct 6, 2009 at 2:13 PM, Kenneth Uildriks <kennethuil at gmail.com>
wrote:
> My front-end is sync'd with the trunk now, and working well, but it
> required some additional functions exposed in the C bindings.  I
> hereby submit them for review and approval for inclusion in the trunk.
>
LLVMGetAttribute had a bug in it.  Here's the revised version of the patch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: cbindings.patch
Type: text/x-patch
Size: 7271 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20091006/91de8d57/attachment.bin>

On Tue, Oct 6, 2009 at 5:47 PM, Kenneth Uildriks <kennethuil at gmail.com>
wrote:
> On Tue, Oct 6, 2009 at 2:13 PM, Kenneth Uildriks <kennethuil at
gmail.com> wrote:
>
> LLVMGetAttribute had a bug in it.  Here's the revised version of the
patch
Hi Kenneth!

I wouldn't say that I'm the best reviewer, but I've been doing some
work with the c bindings recently so hopefully I have some idea of
what I'm talking about :) Comments are inlined:


+/** See the llvm::Use class. */
+typedef struct LLVMOpaqueUse *LLVMUseRef;
+
...
+void LLVMReplaceAllUsesWith(LLVMValueRef OldVal, LLVMValueRef NewVal);
...
+/* Operations on Uses */
+LLVMUseRef LLVMGetFirstUse(LLVMValueRef Val);
+LLVMUseRef LLVMGetNextUse(LLVMUseRef U);
+LLVMValueRef LLVMGetUser(LLVMUseRef U);
+LLVMValueRef LLVMGetUsedValue(LLVMUseRef U);


These seem okay to me, but I don't have too much experience with using
the Use classes. The impression I've gotten from the other developers
is that the C bindings is really designed to just get data into llvm,
and any complex manipulations should really be done in C++ passes.
What's your use case for exposing these more complex manipulations?


+/* Operations on Users */
+LLVMValueRef LLVMGetOperand(LLVMValueRef Val, unsigned Index);


So how are you using this, since you aren't exposing any of the other
operand functionality?


+unsigned long long LLVMConstIntGetZExtValue(LLVMValueRef ConstantVal);
+long long LLVMConstIntGetSExtValue(LLVMValueRef ConstantVal);


I'm not sure about these functions. There really isn't any other way
to get to the value of any other constant, so why do you need this?


 /* Operations on composite constants */
@@ -464,6 +479,7 @@
 LLVMValueRef LLVMConstVector(LLVMValueRef *ScalarConstantVals, unsigned Size);

 /* Constant expressions */
+unsigned LLVMGetConstOpcode(LLVMValueRef ConstantVal);


This seems okay with me, but there really should be an LLVMInstruction
enum defined instead of a raw unsigned value. Could you also add a
LLVMConstExpr that wraps ConstantExpr::get?


+int LLVMHasInitializer(LLVMValueRef GlobalVar);


Seems fine to me. I can commit this now.


+LLVMAttribute LLVMGetFunctionAttr(LLVMValueRef Fn);
+LLVMAttribute LLVMGetAttribute(LLVMValueRef Arg);


I've never really done much with attributes. What are you using this for?

On Oct 6, 2009, at 5:47 PM, Kenneth Uildriks wrote:
> On Tue, Oct 6, 2009 at 2:13 PM, Kenneth Uildriks  
> <kennethuil at gmail.com> wrote:
>> My front-end is sync'd with the trunk now, and working well, but it
>> required some additional functions exposed in the C bindings.  I
>> hereby submit them for review and approval for inclusion in the  
>> trunk.
>>
>
> LLVMGetAttribute had a bug in it.  Here's the revised version of the  
> patch
Hi Kenneth,

Thanks for working on this.  I have some additional comments:


+/** See the llvm::Use class. */
+typedef struct LLVMOpaqueUse *LLVMUseRef;

My understanding is that this actually conceptually corresponds to  
use_iterator, not Use.  Please name this something like  
LLVMUseIterator.  Also, please document this, not just referring to  
llvm::Use.


+int LLVMHasInitializer(LLVMValueRef GlobalVar);
  LLVMValueRef LLVMGetInitializer(LLVMValueRef GlobalVar);

Isn't LLVMHasInitializer just  LLVMGetInitializer(x) != 0?


Otherwise, looks ok to me,

-Chris