ParamAttr Patch - Alignment fix

Anders_Johnsen1 · April 24, 2008, 9:38pm

Hi..

Updated so you now set alignment through LLVMInstrSetAlignment.

Anders Johnsen

Gordon_Henriksen1 · April 26, 2008, 8:02pm

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

Anders_Johnsen1 · April 26, 2008, 9:41pm

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

ParamAttr.patch (5.41 KB)

Gordon_Henriksen1 · April 26, 2008, 10:48pm

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.

Nice. Just a few small formatting nitpicks…

• please stick to 80 columns
• don't place braces on the same line

Index: include/llvm-c/Core.h

--- include/llvm-c/Core.h (revision 50213)
+++ include/llvm-c/Core.h (working copy)
@@ -69,6 +69,7 @@
typedef struct LLVMOpaqueBasicBlock *LLVMBasicBlockRef;
typedef struct LLVMOpaqueBuilder *LLVMBuilderRef;

+

/* Used to provide a module to JIT or interpreter.
* See the llvm::ModuleProvider class.
*/

?

@@ -441,6 +459,9 @@
/* Operations on call sites */
void LLVMSetInstructionCallConv(LLVMValueRef Instr, unsigned CC);
unsigned LLVMGetInstructionCallConv(LLVMValueRef Instr);
+void LLVMInstrAddParamAttr(LLVMValueRef Instr, unsigned index, LLVMParamAttr);
+void LLVMInstrRemoveParamAttr(LLVMValueRef Instr, unsigned index, LLVMParamAttr);
+void LLVMSetInstrParamAlignment(LLVMValueRef Instr, unsigned index, unsigned align);

Could you swap LLVMSetInstrParamAlignment around to be consistent with the others in sentence formation (LLVM<Verb>Instr<Noun>)?

Index: lib/VMCore/Core.cpp

--- lib/VMCore/Core.cpp (revision 50213)
+++ lib/VMCore/Core.cpp (working copy)
+void LLVMSetParamAlignment(LLVMValueRef Arg, unsigned align)
+{
+ unwrap<Argument>(Arg)->addAttr(
+ ParamAttr::constructAlignmentFromInt(align));
+}
+

...

+void LLVMSetInstrParamAlignment(LLVMValueRef Instr, unsigned index, unsigned align) {
+ CallSite Call = CallSite(unwrap<Instruction>(Instr));
+ Call.setParamAttrs(
+ Call.getParamAttrs().addAttr(index,
+ ParamAttr::constructAlignmentFromInt(align)));
+}
+

If I call this twice with different values, don't I get the bitwise OR of the two constructAlignmentFromInt values? Does PAListPtr provide a better API for this?

Thanks Anders!

— Gordon

Anders_Johnsen1 · April 26, 2008, 11:09pm

> 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.

Nice. Just a few small formatting nitpicks…

• please stick to 80 columns
• don't place braces on the same line

> Index: include/llvm-c/Core.h
> ===================================================================
> --- include/llvm-c/Core.h (revision 50213)
> +++ include/llvm-c/Core.h (working copy)
> @@ -69,6 +69,7 @@
> typedef struct LLVMOpaqueBasicBlock *LLVMBasicBlockRef;
> typedef struct LLVMOpaqueBuilder *LLVMBuilderRef;
>
> +
>
> /* Used to provide a module to JIT or interpreter.
> * See the llvm::ModuleProvider class.
> */

?

> @@ -441,6 +459,9 @@
> /* Operations on call sites */
> void LLVMSetInstructionCallConv(LLVMValueRef Instr, unsigned CC);
> unsigned LLVMGetInstructionCallConv(LLVMValueRef Instr);
> +void LLVMInstrAddParamAttr(LLVMValueRef Instr, unsigned index,
> LLVMParamAttr);
> +void LLVMInstrRemoveParamAttr(LLVMValueRef Instr, unsigned index,
> LLVMParamAttr);
> +void LLVMSetInstrParamAlignment(LLVMValueRef Instr, unsigned index,
> unsigned align);

Could you swap LLVMSetInstrParamAlignment around to be consistent with
the others in sentence formation (LLVM<Verb>Instr<Noun>)?

> Index: lib/VMCore/Core.cpp
> ===================================================================
> --- lib/VMCore/Core.cpp (revision 50213)
> +++ lib/VMCore/Core.cpp (working copy)
> +void LLVMSetParamAlignment(LLVMValueRef Arg, unsigned align)
> +{
> + unwrap<Argument>(Arg)->addAttr(
> + ParamAttr::constructAlignmentFromInt(align));
> +}
> +

...

> +void LLVMSetInstrParamAlignment(LLVMValueRef Instr, unsigned index,
> unsigned align) {
> + CallSite Call = CallSite(unwrap<Instruction>(Instr));
> + Call.setParamAttrs(
> + Call.getParamAttrs().addAttr(index,
> + ParamAttr::constructAlignmentFromInt(align)));
> +}
> +

If I call this twice with different values, don't I get the bitwise OR
of the two constructAlignmentFromInt values? Does PAListPtr provide a
better API for this?

You are not allowed to set alignment twice - tried it, and got myself an
assert. (Is this expected behavior?)

Thanks Anders!

— Gordon

_______________________________________________
LLVM Developers mailing list
LLVMdev@cs.uiuc.edu http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

I'll wrap it all up sometime tomorrow.

Thanks,
Anders Johnsen

Gordon_Henriksen1 · April 26, 2008, 11:33pm

Classy!

— Gordon

Anders_Johnsen1 · April 28, 2008, 3:07pm

Should hopefully be the last one

Anders Johnsen

ParamAttr.patch (5.34 KB)

Gordon_Henriksen1 · April 28, 2008, 5:38pm

Looks great. Applied r50360: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20080428/061696.html

Thansk Anders!

— Gordon

Anders_Johnsen1 · April 28, 2008, 7:28pm

Small patch to fix enum values - have been changed while patch have been
updated.

Anders Johnsen

ParamAttrFix.patch (719 Bytes)

Anton_Korobeynikov · April 28, 2008, 9:48pm

Hello, Anders

Small patch to fix enum values - have been changed while patch have
been updated.

Applied, thanks