Hi Gordon,
Thanks a lot for the feedback. I can see I've been way to concentrated on
how
llvm is build, then on this particular patch. I've done the changes you have
suggested and it's now a lot nicer and cleaner!
Please do say, if there is anything else.
Anders Johnsen
On Saturday 26 April 2008 22:02:45 Gordon Henriksen
wrote:> Hi Anders,
>
> Thanks for the patch. I'd like you to incorporate some feedback before
> I apply it, though.
>
> > Index: include/llvm/Argument.h
> > ==================================================================>
> --- include/llvm/Argument.h (revision 50213)
> > +++ include/llvm/Argument.h (working copy)
> > @@ -60,7 +60,16 @@
> > +
> > + /// setByValAttr - Set true to give the Argument a byVal attribute.
> > + void setByValAttr(bool);
> >
> > + /// setNoAliasAttr - Set true to give the Argument a noalias
> > attribute.
> > + void setNoAliasAttr(bool noAlias);
> > +
> > + /// setStructRetAttr - Set true to give the Argument a sret
> > attribute.
> > + void setStructRetAttr(bool byVal);
>
> These prototypes are all misleading; the bool value is ignored. I
> think it would be preferable to simply have a single pair of methods,
> addAttr(ParamAttr Attr) and removeAttr(ParamAttr Attr).
>
> > Index: include/llvm-c/Core.h
> > ==================================================================>
> --- include/llvm-c/Core.h (revision 50213)
> > +++ include/llvm-c/Core.h (working copy)
> > @@ -69,6 +69,8 @@
> > typedef struct LLVMOpaqueBasicBlock *LLVMBasicBlockRef;
> > typedef struct LLVMOpaqueBuilder *LLVMBuilderRef;
> >
> > +typedef struct LLVMOpaqueParamAttrs *LLVMParamAttrs;
>
> Please delete this, as it is unused.
>
> > +void LLVMInstrAddParamAttr(LLVMValueRef, unsigned index,
> > LLVMParamAttr);
> > +void LLVMInstrSetAlignment(LLVMValueRef, unsigned index, unsigned
> > align);
> > +void LLVMInstrRemoveParamAttr(LLVMValueRef, unsigned index,
> > LLVMParamAttr);
> > +void LLVMAddParamAttr(LLVMValueRef, LLVMParamAttr);
> > +void LLVMRemoveParamAttr(LLVMValueRef, LLVMParamAttr);
>
> - Please put these with the things they operate on. The *Instr*
> variants should go with the other functions that take call sites.
> - Name the parameters. This is the only way a user can know from the
> header file what type of LLVMValueRefs are permissible.
> - InstrSetAlignment is a confusing name. (I'd think it refers to the
> alignment for a load or store, with no additional context.) It
> specifically refers to byval parameter alignment, so perhaps
> LLVMSetInstrParamAlignment or LLVMSetInstrByValParamAlignment.
> - How to set alignment on an Argument*?
>
> Finally, how to set or clear attributes like noreturn on a Function*?
> This might indicate that we should use an indexed API for call sites
> as well.
>
> > Index: lib/VMCore/Core.cpp
> > ==================================================================>
> --- lib/VMCore/Core.cpp (revision 50213)
> > +++ lib/VMCore/Core.cpp (working copy)
> > @@ -20,6 +20,7 @@
> > #include "llvm/TypeSymbolTable.h"
> > #include "llvm/ModuleProvider.h"
> > #include "llvm/Support/MemoryBuffer.h"
> > +#include "llvm/Support/CallSite.h"
> > #include <cassert>
> > #include <cstdlib>
> > #include <cstring>
> > @@ -798,6 +799,101 @@
> > return wrap(--I);
> > }
> >
> > +void LLVMAddParamAttr(LLVMValueRef Arg, LLVMParamAttr Param) {
>
> Rename Param -> Attr or PA or something.
>
> > + LLVMParamAttr P = (LLVMParamAttr)Param;
> > + Argument *A = unwrap<Argument>(Arg);
> > +
> > + switch (P) {
> > + case LLVMByValParamAttr:
> > + A->setByValAttr(true);
> > + break;
> > + case LLVMNoAliasParamAttr:
> > + A->setNoAliasAttr(true);
> > + break;
> > + case LLVMStructRetParamAttr:
> > + A->setStructRetAttr(true);
> > + break;
> > + default:
> > + return;
> > + }
> > +}
>
> No need for a switch here; after changing the helpers in Argument,
> just use unwrap<Argument>(Arg)->setAttr(Attr).
>
> > +void LLVMRemoveParamAttr(LLVMValueRef Arg, LLVMParamAttr Param) {
> > + LLVMParamAttr P = (LLVMParamAttr)Param;
> > + Argument *A = unwrap<Argument>(Arg);
> > +
> > + switch (P) {
> > + case LLVMByValParamAttr:
> > + A->setByValAttr(false);
> > + break;
> > + case LLVMNoAliasParamAttr:
> > + A->setNoAliasAttr(false);
> > + break;
> > + case LLVMStructRetParamAttr:
> > + A->setStructRetAttr(false);
> > + break;
> > + default:
> > + return;
> > + }
> > +}
>
> Again.
>
> > +void LLVMInstrAddParamAttr(LLVMValueRef Instr, unsigned index,
> > LLVMParamAttr Param) {
> > +
> > + Instruction *I = unwrap<Instruction>(Instr);
>
> No need to declare a variable for this; it's used only once.
>
> > + CallSite Call = CallSite(I);
> > +
> > + LLVMParamAttr P = (LLVMParamAttr)Param;
> > + unsigned A;
> > + switch (P) {
> > + case LLVMZExtParamAttr: A = ParamAttr::ZExt; break;
> > + case LLVMSExtParamAttr: A = ParamAttr::SExt; break;
> > + case LLVMNoReturnParamAttr: A = ParamAttr::NoReturn; break;
> > + case LLVMNoUnwindParamAttr: A = ParamAttr::NoUnwind; break;
> > + case LLVMInRegParamAttr: A = ParamAttr::InReg; break;
> > + case LLVMNoAliasParamAttr: A = ParamAttr::NoAlias; break;
> > + case LLVMStructRetParamAttr: A = ParamAttr::StructRet; break;
> > + case LLVMByValParamAttr: A = ParamAttr::ByVal; break;
> > + case LLVMNestParamAttr: A = ParamAttr::Nest; break;
> > + case LLVMReadNoneParamAttr: A = ParamAttr::ReadNone; break;
> > + case LLVMReadOnlyParamAttr: A = ParamAttr::ReadOnly; break;
> > + }
> > + Call.setParamAttrs(
> > + Call.getParamAttrs().addAttr(index, A));
> > +}
>
> This switch is unnecessary. Just make LLVMParamAttr equivalent to
> llvm::ParamAttr.
>
> > +void LLVMInstrRemoveParamAttr(LLVMValueRef Instr, unsigned index,
> > LLVMParamAttr Param) {
> > +
> > + Instruction *I = unwrap<Instruction>(Instr);
> > + CallSite Call = CallSite(I);
> > +
> > + LLVMParamAttr P = (LLVMParamAttr)Param;
> > + unsigned A;
> > + switch (P) {
> > + case LLVMZExtParamAttr: A = ParamAttr::ZExt; break;
> > + case LLVMSExtParamAttr: A = ParamAttr::SExt; break;
> > + case LLVMNoReturnParamAttr: A = ParamAttr::NoReturn; break;
> > + case LLVMNoUnwindParamAttr: A = ParamAttr::NoUnwind; break;
> > + case LLVMInRegParamAttr: A = ParamAttr::InReg; break;
> > + case LLVMNoAliasParamAttr: A = ParamAttr::NoAlias; break;
> > + case LLVMStructRetParamAttr: A = ParamAttr::StructRet; break;
> > + case LLVMByValParamAttr: A = ParamAttr::ByVal; break;
> > + case LLVMNestParamAttr: A = ParamAttr::Nest; break;
> > + case LLVMReadNoneParamAttr: A = ParamAttr::ReadNone; break;
> > + case LLVMReadOnlyParamAttr: A = ParamAttr::ReadOnly; break;
> > + }
> > + Call.setParamAttrs(
> > + Call.getParamAttrs().removeAttr(index, A));
> > +}
>
> Same feedback as the previous function.
>
> > +void LLVMInstrSetAlignment(LLVMValueRef Instr, unsigned index,
> > unsigned align) {
> > + Instruction *I = unwrap<Instruction>(Instr);
> > + CallSite Call = CallSite(I);
> > + Call.setParamAttrs(
> > + Call.getParamAttrs().addAttr(index,
> > + llvm::ParamAttr::constructAlignmentFromInt(align)));
> > +}
>
> - Unnecessary variable again.
> - The llvm:: prefix is unnecessary because of the using namespace
> llvm; at the top of the file.
>
> This is good. In general, most bindings should be this order of
> complexity or simpler.
>
> — Gordon
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ParamAttr.patch
Type: text/x-diff
Size: 5535 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20080426/80b4428e/attachment.patch>