[LLVMdev] ParamAttr Patch - Alignment fix

Anders Johnsen skabet at gmail.com
Sat Apr 26 14:41:45 PDT 2008


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>


More information about the llvm-dev mailing list