[PATCH 1/4] sparse, llvm: Fix resulting type of store address calculations

[PATCH 1/4] sparse, llvm: Fix resulting type of store address calculations

[Jonathan Neuschäfer]

Use the fix from d5bd3662 ("sparse, llvm: Fix type of loaded values").

Cc: Pekka Enberg <penberg@kernel.org>
Cc: Christopher Li <sparse@chrisli.org>
Cc: Jeff Garzik <jgarzik@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Xi Wang <xi.wang@gmail.com>
Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 sparse-llvm.c                   |    2 +-
 validation/backend/store-type.c |   12 ++++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)
 create mode 100644 validation/backend/store-type.c

diff --git a/sparse-llvm.c b/sparse-llvm.c
index 6f2fbd6..837a96f 100644
--- a/sparse-llvm.c
+++ b/sparse-llvm.c
@@ -640,7 +640,7 @@ static void output_op_store(struct function *fn, struct instruction *insn)
 
 	/* convert address back to pointer */
 	addr = LLVMBuildIntToPtr(fn->builder, addr_i,
-				 LLVMPointerType(int_type, 0), "addr");
+				 LLVMTypeOf(src_p), "addr");
 
 	target_in = pseudo_to_value(fn, insn, insn->target);
 
diff --git a/validation/backend/store-type.c b/validation/backend/store-type.c
new file mode 100644
index 0000000..9e2ce73
--- /dev/null
+++ b/validation/backend/store-type.c
@@ -0,0 +1,12 @@
+struct foo;
+static struct foo *var;
+
+static void set(struct foo *f)
+{
+       var = f;
+}
+
+/*
+ * check-name: Type of stored objects
+ * check-command: ./sparsec -c $file -o tmp.o
+ */
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] sparse, llvm: de-duplicate load/store address calculation code

[Jonathan Neuschäfer]

Cc: Pekka Enberg <penberg@kernel.org>
Cc: Christopher Li <sparse@chrisli.org>
Cc: Jeff Garzik <jgarzik@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Xi Wang <xi.wang@gmail.com>
Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 sparse-llvm.c |   31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/sparse-llvm.c b/sparse-llvm.c
index 837a96f..8573eda 100644
--- a/sparse-llvm.c
+++ b/sparse-llvm.c
@@ -598,10 +598,10 @@ static void output_op_ret(struct function *fn, struct instruction *insn)
 		LLVMBuildRetVoid(fn->builder);
 }
 
-static void output_op_load(struct function *fn, struct instruction *insn)
+static LLVMValueRef calc_memop_addr(struct function *fn, struct instruction *insn)
 {
 	LLVMTypeRef int_type;
-	LLVMValueRef src_p, src_i, ofs_i, addr_i, addr, target;
+	LLVMValueRef src_p, src_i, ofs_i, addr_i, addr;
 
 	/* int type large enough to hold a pointer */
 	int_type = LLVMIntType(bits_in_pointer);
@@ -617,6 +617,16 @@ static void output_op_load(struct function *fn, struct instruction *insn)
 	addr = LLVMBuildIntToPtr(fn->builder, addr_i,
 				 LLVMTypeOf(src_p), "addr");
 
+	return addr;
+}
+
+
+static void output_op_load(struct function *fn, struct instruction *insn)
+{
+	LLVMValueRef addr, target;
+
+	addr = calc_memop_addr(fn, insn);
+
 	/* perform load */
 	target = LLVMBuildLoad(fn->builder, addr, "load_target");
 
@@ -625,22 +635,9 @@ static void output_op_load(struct function *fn, struct instruction *insn)
 
 static void output_op_store(struct function *fn, struct instruction *insn)
 {
-	LLVMTypeRef int_type;
-	LLVMValueRef src_p, src_i, ofs_i, addr_i, addr, target, target_in;
-
-	/* int type large enough to hold a pointer */
-	int_type = LLVMIntType(bits_in_pointer);
-
-	/* convert to integer, add src + offset */
-	src_p = pseudo_to_value(fn, insn, insn->src);
-	src_i = LLVMBuildPtrToInt(fn->builder, src_p, int_type, "src_i");
+	LLVMValueRef addr, target, target_in;
 
-	ofs_i = LLVMConstInt(int_type, insn->offset, 0);
-	addr_i = LLVMBuildAdd(fn->builder, src_i, ofs_i, "addr_i");

[PATCH 3/4] sparse, llvm: base load/store address type on insn_symbol_type()

[Jonathan Neuschäfer]

LLVM needs to be correctly told about the type of the object
being accessed.

This patch also fixes code generation for struct accesses.

Cc: Pekka Enberg <penberg@kernel.org>
Cc: Christopher Li <sparse@chrisli.org>
Cc: Jeff Garzik <jgarzik@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Xi Wang <xi.wang@gmail.com>
Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 sparse-llvm.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/sparse-llvm.c b/sparse-llvm.c
index 8573eda..a8ebeab 100644
--- a/sparse-llvm.c
+++ b/sparse-llvm.c
@@ -600,7 +600,7 @@ static void output_op_ret(struct function *fn, struct instruction *insn)
 
 static LLVMValueRef calc_memop_addr(struct function *fn, struct instruction *insn)
 {
-	LLVMTypeRef int_type;
+	LLVMTypeRef int_type, addr_type;
 	LLVMValueRef src_p, src_i, ofs_i, addr_i, addr;
 
 	/* int type large enough to hold a pointer */
@@ -613,9 +613,10 @@ static LLVMValueRef calc_memop_addr(struct function *fn, struct instruction *ins
 	ofs_i = LLVMConstInt(int_type, insn->offset, 0);
 	addr_i = LLVMBuildAdd(fn->builder, src_i, ofs_i, "addr_i");
 
+	addr_type = LLVMPointerType(insn_symbol_type(fn->module, insn), 0);
+
 	/* convert address back to pointer */
-	addr = LLVMBuildIntToPtr(fn->builder, addr_i,
-				 LLVMTypeOf(src_p), "addr");
+	addr = LLVMBuildIntToPtr(fn->builder, addr_i, addr_type, "addr");
 
 	return addr;
 }
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] sparse, llvm: base load/store address type on insn_symbol_type()

[Xi Wang]

On 05/18/2013 01:52 PM, Jonathan Neuschäfer wrote:
>   	/* convert address back to pointer */
> -	addr = LLVMBuildIntToPtr(fn->builder, addr_i,
> -				 LLVMTypeOf(src_p), "addr");
> +	addr = LLVMBuildIntToPtr(fn->builder, addr_i, addr_type, "addr");

Actually, we shouldn't convert pointers to integers in the first place.
This effectively disables pointer analysis and future optimizations.

A better way is to use LLVM's GEP for pointer arithmetic, by converting
pointers to `char *', rather than integers.

See more examples here:
http://www.spinics.net/lists/linux-sparse/msg02768.html

Jonathan, how about this version using `char *' based on your patchset?

diff --git a/sparse-llvm.c b/sparse-llvm.c
index 00ace6e..a01c4b7 100644
--- a/sparse-llvm.c
+++ b/sparse-llvm.c
@@ -541,24 +541,47 @@ static void output_op_ret(struct function *fn, struct instruction *insn)
 		LLVMBuildRetVoid(fn->builder);
 }
 
-static void output_op_load(struct function *fn, struct instruction *insn)
+static LLVMValueRef calc_gep(LLVMBuilderRef builder, LLVMValueRef base, LLVMValueRef off)
+{
+	LLVMTypeRef type = LLVMTypeOf(base);
+	unsigned int as = LLVMGetPointerAddressSpace(type);
+	LLVMTypeRef bytep = LLVMPointerType(LLVMInt8Type(), as);
+	LLVMValueRef addr;
+
+	/* convert base to char* type */
+	base = LLVMBuildPointerCast(builder, base, bytep, "");
+	/* addr = base + off */
+	addr = LLVMBuildInBoundsGEP(builder, base, &off, 1, "");
+	/* convert base back to the actual pointer type */
+	addr = LLVMBuildPointerCast(builder, addr, type, "");
+	return addr;
+}
+
+static LLVMValueRef calc_memop_addr(struct function *fn, struct instruction *insn)
 {
-	LLVMTypeRef int_type;
-	LLVMValueRef src_p, src_i, ofs_i, addr_i, addr, target;
+	LLVMTypeRef addr_type, int_type;
+	LLVMValueRef src, base, off, addr;
+	unsigned int as;
+
+	src = pseudo_to_value(fn, insn, insn->src);
+	as = LLVMGetPointerAddressSpace(LLVMTypeOf(src));
+	addr_type = LLVMPointerType(insn_symbol_type(fn->module, insn), as);
+	base = LLVMBuildPointerCast(fn->builder, src, addr_type, "");
 
 	/* int type large enough to hold a pointer */
 	int_type = LLVMIntType(bits_in_pointer);
+	off = LLVMConstInt(int_type, insn->offset, 0);
+
+	addr = calc_gep(fn->builder, base, off);
+	return addr;
+}
 
-	/* convert to integer, add src + offset */
-	src_p = pseudo_to_value(fn, insn, insn->src);
-	src_i = LLVMBuildPtrToInt(fn->builder, src_p, int_type, "src_i");
 
-	ofs_i = LLVMConstInt(int_type, insn->offset, 0);
-	addr_i = LLVMBuildAdd(fn->builder, src_i, ofs_i, "addr_i");
+static void output_op_load(struct function *fn, struct instruction *insn)
+{
+	LLVMValueRef addr, target;
 
-	/* convert address back to pointer */
-	addr = LLVMBuildIntToPtr(fn->builder, addr_i,
-				 LLVMTypeOf(src_p), "addr");
+	addr = calc_memop_addr(fn, insn);
 
 	/* perform load */
 	target = LLVMBuildLoad(fn->builder, addr, "load_target");
@@ -568,22 +591,9 @@ static void output_op_load(struct function *fn, struct instruction *insn)
 
 static void output_op_store(struct function *fn, struct instruction *insn)
 {
-	LLVMTypeRef int_type;
-	LLVMValueRef src_p, src_i, ofs_i, addr_i, addr, target, target_in;
-
-	/* int type large enough to hold a pointer */
-	int_type = LLVMIntType(bits_in_pointer);
-
-	/* convert to integer, add src + offset */
-	src_p = pseudo_to_value(fn, insn, insn->src);
-	src_i = LLVMBuildPtrToInt(fn->builder, src_p, int_type, "src_i");
-
-	ofs_i = LLVMConstInt(int_type, insn->offset, 0);
-	addr_i = LLVMBuildAdd(fn->builder, src_i, ofs_i, "addr_i");
+	LLVMValueRef addr, target, target_in;
 
-	/* convert address back to pointer */
-	addr = LLVMBuildIntToPtr(fn->builder, addr_i,
-				 LLVMPointerType(int_type, 0), "addr");
+	addr = calc_memop_addr(fn, insn);
 
 	target_in = pseudo_to_value(fn, insn, insn->target);
 
diff --git a/validation/backend/store-type.c b/validation/backend/store-type.c
new file mode 100644
index 0000000..9e2ce73
--- /dev/null
+++ b/validation/backend/store-type.c
@@ -0,0 +1,12 @@
+struct foo;
+static struct foo *var;
+
+static void set(struct foo *f)
+{
+       var = f;
+}
+
+/*
+ * check-name: Type of stored objects
+ * check-command: ./sparsec -c $file -o tmp.o
+ */
diff --git a/validation/backend/struct-access.c b/validation/backend/struct-access.c
new file mode 100644
index 0000000..884b470
--- /dev/null
+++ b/validation/backend/struct-access.c
@@ -0,0 +1,28 @@
+struct st {
+	int i, *d;
+};
+
+static int load_i(struct st *st)
+{
+	return st->i;
+}
+
+static void store_i(struct st *st, int i)
+{
+	st->i = i;
+}
+
+static int *load_d(struct st *st)
+{
+	return st->d;
+}
+
+static void store_d(struct st *st, int *d)
+{
+	st->d = d;
+}
+
+/*
+ * check-name: struct access code generation
+ * check-command: ./sparsec -c $file -o tmp.o
+ */
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] sparse, llvm: base load/store address type on insn_symbol_type()

[Jonathan Neuschäfer]

On Sat, May 18, 2013 at 06:45:18PM -0400, Xi Wang wrote:
> On 05/18/2013 01:52 PM, Jonathan Neuschäfer wrote:
> >   	/* convert address back to pointer */
> > -	addr = LLVMBuildIntToPtr(fn->builder, addr_i,
> > -				 LLVMTypeOf(src_p), "addr");
> > +	addr = LLVMBuildIntToPtr(fn->builder, addr_i, addr_type, "addr");
> 
> Actually, we shouldn't convert pointers to integers in the first place.
> This effectively disables pointer analysis and future optimizations.
> 
> A better way is to use LLVM's GEP for pointer arithmetic, by converting
> pointers to `char *', rather than integers.
> 
> See more examples here:
> http://www.spinics.net/lists/linux-sparse/msg02768.html
> 
> Jonathan, how about this version using `char *' based on your patchset?

It looks good. ACK.

> +	LLVMTypeRef type = LLVMTypeOf(base);
> +	unsigned int as = LLVMGetPointerAddressSpace(type);

Right, I was sloppy about address spaces. :-)


Thanks,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] sparse, llvm: add a struct access test case

[Jonathan Neuschäfer]

Cc: Pekka Enberg <penberg@kernel.org>
Cc: Christopher Li <sparse@chrisli.org>
Cc: Jeff Garzik <jgarzik@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Xi Wang <xi.wang@gmail.com>
Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 validation/backend/struct-access.c |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 validation/backend/struct-access.c

diff --git a/validation/backend/struct-access.c b/validation/backend/struct-access.c
new file mode 100644
index 0000000..884b470
--- /dev/null
+++ b/validation/backend/struct-access.c
@@ -0,0 +1,28 @@
+struct st {
+	int i, *d;
+};
+
+static int load_i(struct st *st)
+{
+	return st->i;
+}
+
+static void store_i(struct st *st, int i)
+{
+	st->i = i;
+}
+
+static int *load_d(struct st *st)
+{
+	return st->d;
+}
+
+static void store_d(struct st *st, int *d)
+{
+	st->d = d;
+}
+
+/*
+ * check-name: struct access code generation
+ * check-command: ./sparsec -c $file -o tmp.o
+ */
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] sparse, llvm: Fix resulting type of store address calculations

[Pekka Enberg]

Applied all four patches, thanks a lot Jonathan!