[LLVMdev] ParamAttr Patch - Alignment fix

[Gordon Henriksen]

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