[WIP 00/13] LLVM fixes

[WIP 00/13] LLVM fixes

[Luc Van Oostenryck]

This series solves a number of issues in sparse-llvm,
mainly about wrong type information as needed for LLVM
or simply the lack of these infos.
These issues have been reported and investigated by
Dibyendu Majumdar.

These patches have been lightly tested and already
allow to compile a lot more code to LLVM.
It's still incomplete though:
- it won't work on bitfields
- it won't work on computed gotos
- it won't work on label-as-value
- it won't work on exotic instructions (OP_SPLICE)
- few things are working correctly with floats
  (but this is not specific to sparse-llvm).
There is most probably a bunch of others issues too.

For testing purpose this serie is also available at:
  https://github.com/lucvoo/sparse/tree/llvm-fixes-v0


Luc Van Oostenryck (13):
  llvm: add a helper to convert an integer to a ValueRef
  llvm: fix translation of PSEUDO_VALs into a ValueRefs
  llvm: fix output_op_store() which modify its operand
  llvm: fix output_op_[ptr]cast()
  add get_nth1_arg()
  llvm: fix type of literal integer passed as arguments
  llvm: fix output OP_ADD mixed with pointers
  llvm: add support for OP_NEG
  give a type to OP_PHISOURCE
  give a type to OP_SEL, always
  llvm: remove unneeded arg 'module'
  llvm: remove unneeded arg 'fn'
  llvm: fix: do not mix pointers and floats when doing compares

 flow.c                        |   2 +-
 linearize.c                   |  18 ++--
 linearize.h                   |   2 +-
 memops.c                      |   2 +-
 sparse-llvm.c                 | 187 +++++++++++++++++++++++++++++++-----------
 symbol.h                      |  13 +++
 validation/backend/null.c     |  24 ++++++
 validation/backend/store-x2.c |  16 ++++
 8 files changed, 203 insertions(+), 61 deletions(-)
 create mode 100644 validation/backend/null.c
 create mode 100644 validation/backend/store-x2.c

-- 
2.11.1

[PATCH 01/13] llvm: add a helper to convert an integer to a ValueRef

[Luc Van Oostenryck]

This needs to take in account the fact that the value can have
a non-integer type, for example it can be an address constant.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 sparse-llvm.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/sparse-llvm.c b/sparse-llvm.c
index 9f362b3ed..41ee1fa1f 100644
--- a/sparse-llvm.c
+++ b/sparse-llvm.c
@@ -303,6 +303,30 @@ static void pseudo_name(pseudo_t pseudo, char *buf)
 	}
 }
 
+static LLVMValueRef val_to_value(struct function *fn, unsigned long long val, struct symbol *ctype)
+{
+	LLVMTypeRef dtype;
+	LLVMTypeRef itype;
+	LLVMValueRef result;
+
+	assert(ctype);
+	dtype = symbol_type(fn->module, ctype);
+	switch (LLVMGetTypeKind(dtype)) {
+	case LLVMPointerTypeKind:
+		itype = LLVMIntType(bits_in_pointer);
+		result = LLVMConstInt(itype, val, 1);
+		result = LLVMConstIntToPtr(result, dtype);
+		break;
+	case LLVMIntegerTypeKind:
+		result = LLVMConstInt(dtype, val, 1);
+		break;
+	default:
+		assert(0);
+	}
+
+	return result;
+}
+
 static LLVMValueRef pseudo_to_value(struct function *fn, struct instruction *insn, pseudo_t pseudo)
 {
 	LLVMValueRef result = NULL;
-- 
2.11.1

[PATCH 02/13] llvm: fix translation of PSEUDO_VALs into a ValueRefs

[Luc Van Oostenryck]

In sparse-llvm there is the assumption that a PSEUDO_VAL is always
of integer type. But this is not always the case: constant pointers,
like NULL, are also of the PSEUDO_VAL kind.

Fix this by using the newly created helper val_to_value() and using the
instruction's type where this pseudo is used as the type of the value.

Note: while this patch improve the situation, like for example for the
test cases added here, it's still not correct because now we're making
the assumption that 'insn->type' is the type we need for the pseudo.
This is often true, but certainly not always.
For example this is not true for:
- OP_STORE/OP_LOAD's insn->src
- OP_SET{EQ,...}'s   insn->src[12]
- probably some  others ones
- in general, obviously, for any instructions where the target has
  a different type than the operands.

Reported-by: Dibyendu Majumdar <mobile@majumdar.org.uk>
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 sparse-llvm.c             |  2 +-
 validation/backend/null.c | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)
 create mode 100644 validation/backend/null.c

diff --git a/sparse-llvm.c b/sparse-llvm.c
index 41ee1fa1f..d48b3b20a 100644
--- a/sparse-llvm.c
+++ b/sparse-llvm.c
@@ -384,7 +384,7 @@ static LLVMValueRef pseudo_to_value(struct function *fn, struct instruction *ins
 		break;
 	}
 	case PSEUDO_VAL:
-		result = LLVMConstInt(insn_symbol_type(fn->module, insn), pseudo->value, 1);
+		result = val_to_value(fn, pseudo->value, insn->type);
 		break;
 	case PSEUDO_ARG: {
 		result = LLVMGetParam(fn->fn, pseudo->nr - 1);
diff --git a/validation/backend/null.c b/validation/backend/null.c
new file mode 100644
index 000000000..5c595c70b
--- /dev/null
+++ b/validation/backend/null.c
@@ -0,0 +1,24 @@
+extern int *ip[];
+
+void foo(void);
+void foo(void)
+{
+	ip[0] = (void *)0L;
+	ip[1] = (int *)0L;
+	ip[2] = (void *)0;
+	ip[3] = (int *)0;
+	ip[4] = (void *)(long)0;
+	ip[5] = (int *)(long)0;
+	ip[6] = (void *)123;
+	ip[7] = (int *)123;
+	ip[8] = (void *)123L;
+	ip[9] = (int *)123L;
+	ip[10] = (void *)(long)123;
+	ip[11] = (int *)(long)123;
+}
+
+/*
+ * check-name: store constants to pointer
+ * check-command: sparse-llvm $file
+ * check-output-ignore
+ */
-- 
2.11.1

[PATCH 03/13] llvm: fix output_op_store() which modify its operand

[Luc Van Oostenryck]

In sparse-llvm the field 'priv' of a pseudo is used to store
the corresponding LLVMValueRef. This field is normaly assigned
when processing the instruction that produces the speudo.

In output_op_store(), the field insn->target->priv is overwritten
by the LLVMValueRef returned by LLVMBuildStore().
It's unclear what this return value is:
- this corrupts the pseudo, making it unusable in subsequent
  instructions.
- there is no reason to change this field anyway.

Fix this by removing the assignment to insn->target->priv
in output_op_store().

Reported-by: Dibyendu Majumdar <mobile@majumdar.org.uk>
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 sparse-llvm.c                 |  6 ++----
 validation/backend/store-x2.c | 16 ++++++++++++++++
 2 files changed, 18 insertions(+), 4 deletions(-)
 create mode 100644 validation/backend/store-x2.c

diff --git a/sparse-llvm.c b/sparse-llvm.c
index d48b3b20a..665b6c7dc 100644
--- a/sparse-llvm.c
+++ b/sparse-llvm.c
@@ -646,16 +646,14 @@ static void output_op_load(struct function *fn, struct instruction *insn)
 
 static void output_op_store(struct function *fn, struct instruction *insn)
 {
-	LLVMValueRef addr, target, target_in;
+	LLVMValueRef addr, target_in;
 
 	addr = calc_memop_addr(fn, insn);
 
 	target_in = pseudo_to_value(fn, insn, insn->target);
 
 	/* perform store */
-	target = LLVMBuildStore(fn->builder, target_in, addr);
-
-	insn->target->priv = target;
+	LLVMBuildStore(fn->builder, target_in, addr);
 }
 
 static LLVMValueRef bool_value(struct function *fn, LLVMValueRef value)
diff --git a/validation/backend/store-x2.c b/validation/backend/store-x2.c
new file mode 100644
index 000000000..5ccc9b43a
--- /dev/null
+++ b/validation/backend/store-x2.c
@@ -0,0 +1,16 @@
+void foo(int *p, int a, int b);
+void foo(int *p, int a, int b)
+{
+	int c = a + b;
+
+	p[0] = c;
+	p[1] = c;
+}
+
+/*
+ * check-name: store-x2
+ * check-command: sparsec -c $file -o tmp.o
+ * check-description: Verify in output_op_store() that
+ *	the first store doesn't mess anymore with the
+ *	'target' and thus making the second store unusable.
+ */
-- 
2.11.1

[PATCH 04/13] llvm: fix output_op_[ptr]cast()

[Luc Van Oostenryck]

OP_PTRCASTs can't always be directly translated into LLVM bitcasts and
OP_[S]CASTs can't always be translated into LLVM's trunc/sext/zext
because integer to pointer and pointer to integer must be handled too.

Fix this in output_op_ptrcast() & output_op_cast() by issuing
LLVMBuildIntToPtr/PtrToInt when appropriate.

Reported-by: Dibyendu Majumdar <mobile@majumdar.org.uk>
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 sparse-llvm.c | 40 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/sparse-llvm.c b/sparse-llvm.c
index 665b6c7dc..c593f831f 100644
--- a/sparse-llvm.c
+++ b/sparse-llvm.c
@@ -787,6 +787,8 @@ static void output_op_phi(struct function *fn, struct instruction *insn)
 static void output_op_ptrcast(struct function *fn, struct instruction *insn)
 {
 	LLVMValueRef src, target;
+	LLVMTypeRef dtype;
+	LLVMOpcode op;
 	char target_name[64];
 
 	src = insn->src->priv;
@@ -797,15 +799,31 @@ static void output_op_ptrcast(struct function *fn, struct instruction *insn)
 
 	assert(!symbol_is_fp_type(insn->type));
 
-	target = LLVMBuildBitCast(fn->builder, src, insn_symbol_type(fn->module, insn), target_name);
+	dtype = insn_symbol_type(fn->module, insn);
+	switch (LLVMGetTypeKind(LLVMTypeOf(src))) {
+	case LLVMPointerTypeKind:
+		op = LLVMBitCast;
+		break;
+	case LLVMIntegerTypeKind:
+		op = LLVMIntToPtr;
+		break;
+	default:
+		assert(0);
+	}
 
+	target = LLVMBuildCast(fn->builder, op, src, dtype, target_name);
 	insn->target->priv = target;
 }
 
 static void output_op_cast(struct function *fn, struct instruction *insn, LLVMOpcode op)
 {
 	LLVMValueRef src, target;
+	LLVMTypeRef dtype;
 	char target_name[64];
+	unsigned int width;
+
+	if (is_ptr_type(insn->type))
+		return output_op_ptrcast(fn, insn);
 
 	src = insn->src->priv;
 	if (!src)
@@ -815,11 +833,23 @@ static void output_op_cast(struct function *fn, struct instruction *insn, LLVMOp
 
 	assert(!symbol_is_fp_type(insn->type));
 
-	if (insn->size < LLVMGetIntTypeWidth(LLVMTypeOf(src)))
-		target = LLVMBuildTrunc(fn->builder, src, insn_symbol_type(fn->module, insn), target_name);
-	else
-		target = LLVMBuildCast(fn->builder, op, src, insn_symbol_type(fn->module, insn), target_name);
+	dtype = insn_symbol_type(fn->module, insn);
+	switch (LLVMGetTypeKind(LLVMTypeOf(src))) {
+	case LLVMPointerTypeKind:
+		op = LLVMPtrToInt;
+		break;
+	case LLVMIntegerTypeKind:
+		width = LLVMGetIntTypeWidth(LLVMTypeOf(src));
+		if (insn->size < width)
+			op = LLVMTrunc;
+		else if (insn->size == width)
+			op = LLVMBitCast;
+		break;
+	default:
+		assert(0);
+	}
 
+	target = LLVMBuildCast(fn->builder, op, src, dtype, target_name);
 	insn->target->priv = target;
 }
 
-- 
2.11.1

[PATCH 05/13] add get_nth1_arg()

[Luc Van Oostenryck]

A backend may need to know the size or the type of an
argument and there is no easy way to access to this info.

Fix this by adding an helper returning the symbol associated
to the nth argument (starting at 1).

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 symbol.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/symbol.h b/symbol.h
index 36f8345b5..2bc0e21a0 100644
--- a/symbol.h
+++ b/symbol.h
@@ -416,6 +416,19 @@ static inline int get_sym_type(struct symbol *type)
 	return type->type;
 }
 
+static inline struct symbol *get_nth1_arg(struct symbol *fn, int idx)
+{
+	struct symbol_list *args = fn->ctype.base_type->arguments;
+	struct symbol *arg;
+	int i = 0;
+	FOR_EACH_PTR(args, arg) {
+		if (++i == idx)
+			return arg;
+	} END_FOR_EACH_PTR(arg);
+
+	return NULL;
+}
+
 static inline struct symbol *lookup_keyword(struct ident *ident, enum namespace ns)
 {
 	if (!ident->keyword)
-- 
2.11.1

[PATCH 06/13] llvm: fix type of literal integer passed as arguments

[Luc Van Oostenryck]

Like for all others instructions, LLVM needs the type
of each operands. However this information is not always
available via the pseudo, like here when passing a integer
constant as argument since for sparse constants are typeless.

Fix this by getting the type via the function prototype.

Reported-by: Dibyendu Majumdar <mobile@majumdar.org.uk>
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 sparse-llvm.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/sparse-llvm.c b/sparse-llvm.c
index c593f831f..27cc1b88c 100644
--- a/sparse-llvm.c
+++ b/sparse-llvm.c
@@ -740,7 +740,16 @@ static void output_op_call(struct function *fn, struct instruction *insn)
 
 	i = 0;
 	FOR_EACH_PTR(insn->arguments, arg) {
-		args[i++] = pseudo_to_value(fn, insn, arg);
+		LLVMValueRef value;
+		if (arg->type == PSEUDO_VAL) {
+			/* Value pseudos do not have type information. */
+			/* Use the function prototype to get the type. */
+			struct symbol *ctype = get_nth1_arg(insn->func->sym, i + 1);
+			value = val_to_value(fn, arg->value, ctype);
+		} else {
+			value = pseudo_to_value(fn, insn, arg);
+		}
+		args[i++] = value;
 	} END_FOR_EACH_PTR(arg);
 
 	func = pseudo_to_value(fn, insn, insn->func);
-- 
2.11.1

[PATCH 07/13] llvm: fix output OP_ADD mixed with pointers

[Luc Van Oostenryck]

In sparse, pointer arithmetic and accessing the field
of a structure or an array is simply done via OP_ADD,
the offset being calculated at evaluation time.
On the other hand, LLVM allows addition only on two
integers and pointer arithmetic/member access is done
via 'getelementptr'.

sparse-llvm didn't took this in account which resulted
in type error in 'add' instructions.

Fix this by catching addition involving pointer and issuing
a getelementptr' instruction for these.

Originally-by: Dibyendu Majumdar <mobile@majumdar.org.uk>
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 sparse-llvm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sparse-llvm.c b/sparse-llvm.c
index 27cc1b88c..ee374b217 100644
--- a/sparse-llvm.c
+++ b/sparse-llvm.c
@@ -472,6 +472,10 @@ static void output_op_binary(struct function *fn, struct instruction *insn)
 	case OP_ADD:
 		if (symbol_is_fp_type(insn->type))
 			target = LLVMBuildFAdd(fn->builder, lhs, rhs, target_name);
+		else if (LLVMGetTypeKind(LLVMTypeOf(lhs)) == LLVMPointerTypeKind)
+			target = LLVMBuildGEP(fn->builder, lhs, &rhs, 1, "");
+		else if (LLVMGetTypeKind(LLVMTypeOf(rhs)) == LLVMPointerTypeKind)
+			target = LLVMBuildGEP(fn->builder, rhs, &lhs, 1, "");
 		else
 			target = LLVMBuildAdd(fn->builder, lhs, rhs, target_name);
 		break;
-- 
2.11.1

[PATCH 08/13] llvm: add support for OP_NEG

[Luc Van Oostenryck]

sparse-llvm has not yet support for OP_NEG (which
need to be done via a subtraction from zero).

Fix this by issuing the appropriate substraction.

CC: Dibyendu Majumdar <mobile@majumdar.org.uk>
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 sparse-llvm.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/sparse-llvm.c b/sparse-llvm.c
index ee374b217..c489009a6 100644
--- a/sparse-llvm.c
+++ b/sparse-llvm.c
@@ -951,9 +951,22 @@ static void output_insn(struct function *fn, struct instruction *insn)
 		insn->target->priv = target;
 		break;
 	}
-	case OP_NEG:
-		assert(0);
+	case OP_NEG: {
+		LLVMValueRef src, target;
+		char target_name[64];
+
+		src = pseudo_to_value(fn, insn, insn->src);
+
+		pseudo_name(insn->target, target_name);
+
+		if (symbol_is_fp_type(insn->type))
+			target = LLVMBuildFNeg(fn->builder, src, target_name);
+		else
+			target = LLVMBuildNeg(fn->builder, src, target_name);
+
+		insn->target->priv = target;
 		break;
+	}
 	case OP_CONTEXT:
 		assert(0);
 		break;
-- 
2.11.1

[PATCH 09/13] give a type to OP_PHISOURCE

[Luc Van Oostenryck]

Currently, OP_PHISOURCEs are given a size but not a type.

There is no good reasons fro that and it complicate the
further correct processing or make it impossible.

CC: Dibyendu Majumdar <mobile@majumdar.org.uk>
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 flow.c      |  2 +-
 linearize.c | 16 +++++++---------
 linearize.h |  2 +-
 memops.c    |  2 +-
 4 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/flow.c b/flow.c
index 7730e70f1..e574d5e98 100644
--- a/flow.c
+++ b/flow.c
@@ -370,7 +370,7 @@ found_dominator:
 		if (dominators && phisrc_in_bb(*dominators, parent))
 			continue;
 		br = delete_last_instruction(&parent->insns);
-		phi = alloc_phi(parent, one->target, one->size);
+		phi = alloc_phi(parent, one->target, one->type);
 		phi->ident = phi->ident ? : pseudo->ident;
 		add_instruction(&parent->insns, br);
 		use_pseudo(insn, phi, add_pseudo(dominators, phi));
diff --git a/linearize.c b/linearize.c
index bdc85beb9..fb4c7bd10 100644
--- a/linearize.c
+++ b/linearize.c
@@ -815,9 +815,9 @@ static pseudo_t argument_pseudo(struct entrypoint *ep, int nr)
 	return pseudo;
 }
 
-pseudo_t alloc_phi(struct basic_block *source, pseudo_t pseudo, int size)
+pseudo_t alloc_phi(struct basic_block *source, pseudo_t pseudo, struct symbol *type)
 {
-	struct instruction *insn = alloc_instruction(OP_PHISOURCE, size);
+	struct instruction *insn = alloc_typed_instruction(OP_PHISOURCE, type);
 	pseudo_t phi = __alloc_pseudo(0);
 	static int nr = 0;
 
@@ -1350,19 +1350,18 @@ static pseudo_t linearize_short_conditional(struct entrypoint *ep, struct expres
 	struct basic_block *bb_false;
 	struct basic_block *merge = alloc_basic_block(ep, expr->pos);
 	pseudo_t phi1, phi2;
-	int size = type_size(expr->ctype);
 
 	if (!expr_false || !ep->active)
 		return VOID;
 
 	bb_false = alloc_basic_block(ep, expr_false->pos);
 	src1 = linearize_expression(ep, cond);
-	phi1 = alloc_phi(ep->active, src1, size);
+	phi1 = alloc_phi(ep->active, src1, expr->ctype);
 	add_branch(ep, expr, src1, merge, bb_false);
 
 	set_activeblock(ep, bb_false);
 	src2 = linearize_expression(ep, expr_false);
-	phi2 = alloc_phi(ep->active, src2, size);
+	phi2 = alloc_phi(ep->active, src2, expr->ctype);
 	set_activeblock(ep, merge);
 
 	return add_join_conditional(ep, expr, phi1, phi2);
@@ -1376,7 +1375,6 @@ static pseudo_t linearize_conditional(struct entrypoint *ep, struct expression *
 	pseudo_t src1, src2;
 	pseudo_t phi1, phi2;
 	struct basic_block *bb_true, *bb_false, *merge;
-	int size = type_size(expr->ctype);
 
 	if (!cond || !expr_true || !expr_false || !ep->active)
 		return VOID;
@@ -1388,12 +1386,12 @@ static pseudo_t linearize_conditional(struct entrypoint *ep, struct expression *
 
 	set_activeblock(ep, bb_true);
 	src1 = linearize_expression(ep, expr_true);
-	phi1 = alloc_phi(ep->active, src1, size);
+	phi1 = alloc_phi(ep->active, src1, expr->ctype);
 	add_goto(ep, merge); 
 
 	set_activeblock(ep, bb_false);
 	src2 = linearize_expression(ep, expr_false);
-	phi2 = alloc_phi(ep->active, src2, size);
+	phi2 = alloc_phi(ep->active, src2, expr->ctype);
 	set_activeblock(ep, merge);
 
 	return add_join_conditional(ep, expr, phi1, phi2);
@@ -1875,7 +1873,7 @@ static pseudo_t linearize_return(struct entrypoint *ep, struct statement *stmt)
 			phi_node->bb = bb_return;
 			add_instruction(&bb_return->insns, phi_node);
 		}
-		phi = alloc_phi(active, src, type_size(expr->ctype));
+		phi = alloc_phi(active, src, expr->ctype);
 		phi->ident = &return_ident;
 		use_pseudo(phi_node, phi, add_pseudo(&phi_node->phi_list, phi));
 	}
diff --git a/linearize.h b/linearize.h
index bac82d7ff..c03940eea 100644
--- a/linearize.h
+++ b/linearize.h
@@ -331,7 +331,7 @@ struct entrypoint {
 extern void insert_select(struct basic_block *bb, struct instruction *br, struct instruction *phi, pseudo_t if_true, pseudo_t if_false);
 extern void insert_branch(struct basic_block *bb, struct instruction *br, struct basic_block *target);
 
-pseudo_t alloc_phi(struct basic_block *source, pseudo_t pseudo, int size);
+pseudo_t alloc_phi(struct basic_block *source, pseudo_t pseudo, struct symbol *type);
 pseudo_t alloc_pseudo(struct instruction *def);
 pseudo_t value_pseudo(long long val);
 
diff --git a/memops.c b/memops.c
index 5efdd6f2d..187a63284 100644
--- a/memops.c
+++ b/memops.c
@@ -52,7 +52,7 @@ no_dominance:
 
 found_dominator:
 		br = delete_last_instruction(&parent->insns);
-		phi = alloc_phi(parent, one->target, one->size);
+		phi = alloc_phi(parent, one->target, one->type);
 		phi->ident = phi->ident ? : one->target->ident;
 		add_instruction(&parent->insns, br);
 		use_pseudo(insn, phi, add_pseudo(dominators, phi));
-- 
2.11.1

[PATCH 10/13] give a type to OP_SEL, always

[Luc Van Oostenryck]

Currently, when a phi-node is converted into a OP_SEL
this instruction is given a size but not a type.

There is no good reasons for that and it complicate the
further correct processing or make it impossible.

CC: Dibyendu Majumdar <mobile@majumdar.org.uk>
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 linearize.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linearize.c b/linearize.c
index fb4c7bd10..03248dd39 100644
--- a/linearize.c
+++ b/linearize.c
@@ -679,7 +679,7 @@ void insert_select(struct basic_block *bb, struct instruction *br, struct instru
 	/* Remove the 'br' */
 	delete_last_instruction(&bb->insns);
 
-	select = alloc_instruction(OP_SEL, phi_node->size);
+	select = alloc_typed_instruction(OP_SEL, phi_node->type);
 	select->bb = bb;
 
 	assert(br->cond);
-- 
2.11.1

[PATCH 11/13] llvm: remove unneeded arg 'module'

[Luc Van Oostenryck]

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 sparse-llvm.c | 72 +++++++++++++++++++++++++++++------------------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/sparse-llvm.c b/sparse-llvm.c
index c489009a6..35562e570 100644
--- a/sparse-llvm.c
+++ b/sparse-llvm.c
@@ -34,14 +34,14 @@ static inline bool symbol_is_fp_type(struct symbol *sym)
 	return sym->ctype.base_type == &fp_type;
 }
 
-static LLVMTypeRef symbol_type(LLVMModuleRef module, struct symbol *sym);
+static LLVMTypeRef symbol_type(struct symbol *sym);
 
-static LLVMTypeRef func_return_type(LLVMModuleRef module, struct symbol *sym)
+static LLVMTypeRef func_return_type(struct symbol *sym)
 {
-	return symbol_type(module, sym->ctype.base_type);
+	return symbol_type(sym->ctype.base_type);
 }
 
-static LLVMTypeRef sym_func_type(LLVMModuleRef module, struct symbol *sym)
+static LLVMTypeRef sym_func_type(struct symbol *sym)
 {
 	LLVMTypeRef *arg_type;
 	LLVMTypeRef func_type;
@@ -55,7 +55,7 @@ static LLVMTypeRef sym_func_type(LLVMModuleRef module, struct symbol *sym)
 	 * symbol declaration info.
 	 */
 
-	ret_type = func_return_type(module, sym);
+	ret_type = func_return_type(sym);
 
 	/* count args, build argument type information */
 	FOR_EACH_PTR(sym->arguments, arg) {
@@ -68,7 +68,7 @@ static LLVMTypeRef sym_func_type(LLVMModuleRef module, struct symbol *sym)
 	FOR_EACH_PTR(sym->arguments, arg) {
 		struct symbol *arg_sym = arg->ctype.base_type;
 
-		arg_type[idx++] = symbol_type(module, arg_sym);
+		arg_type[idx++] = symbol_type(arg_sym);
 	} END_FOR_EACH_PTR(arg);
 	func_type = LLVMFunctionType(ret_type, arg_type, n_arg,
 				     sym->variadic);
@@ -76,7 +76,7 @@ static LLVMTypeRef sym_func_type(LLVMModuleRef module, struct symbol *sym)
 	return func_type;
 }
 
-static LLVMTypeRef sym_array_type(LLVMModuleRef module, struct symbol *sym)
+static LLVMTypeRef sym_array_type(struct symbol *sym)
 {
 	LLVMTypeRef elem_type;
 	struct symbol *base_type;
@@ -85,7 +85,7 @@ static LLVMTypeRef sym_array_type(LLVMModuleRef module, struct symbol *sym)
 	/* empty struct is undefined [6.7.2.1(8)] */
 	assert(base_type->bit_size > 0);
 
-	elem_type = symbol_type(module, base_type);
+	elem_type = symbol_type(base_type);
 	if (!elem_type)
 		return NULL;
 
@@ -94,7 +94,7 @@ static LLVMTypeRef sym_array_type(LLVMModuleRef module, struct symbol *sym)
 
 #define MAX_STRUCT_MEMBERS 64
 
-static LLVMTypeRef sym_struct_type(LLVMModuleRef module, struct symbol *sym)
+static LLVMTypeRef sym_struct_type(struct symbol *sym)
 {
 	LLVMTypeRef elem_types[MAX_STRUCT_MEMBERS];
 	struct symbol *member;
@@ -112,7 +112,7 @@ static LLVMTypeRef sym_struct_type(LLVMModuleRef module, struct symbol *sym)
 
 		assert(nr < MAX_STRUCT_MEMBERS);
 
-		member_type = symbol_type(module, member);
+		member_type = symbol_type(member);
 
 		elem_types[nr++] = member_type; 
 	} END_FOR_EACH_PTR(member);
@@ -121,7 +121,7 @@ static LLVMTypeRef sym_struct_type(LLVMModuleRef module, struct symbol *sym)
 	return ret;
 }
 
-static LLVMTypeRef sym_union_type(LLVMModuleRef module, struct symbol *sym)
+static LLVMTypeRef sym_union_type(struct symbol *sym)
 {
 	LLVMTypeRef elements;
 	unsigned union_size;
@@ -138,7 +138,7 @@ static LLVMTypeRef sym_union_type(LLVMModuleRef module, struct symbol *sym)
 	return LLVMStructType(&elements, 1, 0 /* packed? */);
 }
 
-static LLVMTypeRef sym_ptr_type(LLVMModuleRef module, struct symbol *sym)
+static LLVMTypeRef sym_ptr_type(struct symbol *sym)
 {
 	LLVMTypeRef type;
 
@@ -146,7 +146,7 @@ static LLVMTypeRef sym_ptr_type(LLVMModuleRef module, struct symbol *sym)
 	if (is_void_type(sym->ctype.base_type))
 		type = LLVMInt8Type();
 	else
-		type = symbol_type(module, sym->ctype.base_type);
+		type = symbol_type(sym->ctype.base_type);
 
 	return LLVMPointerType(type, 0);
 }
@@ -199,13 +199,13 @@ static LLVMTypeRef sym_basetype_type(struct symbol *sym)
 	return ret;
 }
 
-static LLVMTypeRef symbol_type(LLVMModuleRef module, struct symbol *sym)
+static LLVMTypeRef symbol_type(struct symbol *sym)
 {
 	LLVMTypeRef ret = NULL;
 
 	/* don't cache the result for SYM_NODE */
 	if (sym->type == SYM_NODE)
-		return symbol_type(module, sym->ctype.base_type);
+		return symbol_type(sym->ctype.base_type);
 
 	if (sym->aux)
 		return sym->aux;
@@ -213,25 +213,25 @@ static LLVMTypeRef symbol_type(LLVMModuleRef module, struct symbol *sym)
 	switch (sym->type) {
 	case SYM_BITFIELD:
 	case SYM_ENUM:
-		ret = symbol_type(module, sym->ctype.base_type);
+		ret = symbol_type(sym->ctype.base_type);
 		break;
 	case SYM_BASETYPE:
 		ret = sym_basetype_type(sym);
 		break;
 	case SYM_PTR:
-		ret = sym_ptr_type(module, sym);
+		ret = sym_ptr_type(sym);
 		break;
 	case SYM_UNION:
-		ret = sym_union_type(module, sym);
+		ret = sym_union_type(sym);
 		break;
 	case SYM_STRUCT:
-		ret = sym_struct_type(module, sym);
+		ret = sym_struct_type(sym);
 		break;
 	case SYM_ARRAY:
-		ret = sym_array_type(module, sym);
+		ret = sym_array_type(sym);
 		break;
 	case SYM_FN:
-		ret = sym_func_type(module, sym);
+		ret = sym_func_type(sym);
 		break;
 	default:
 		assert(0);
@@ -242,10 +242,10 @@ static LLVMTypeRef symbol_type(LLVMModuleRef module, struct symbol *sym)
 	return ret;
 }
 
-static LLVMTypeRef insn_symbol_type(LLVMModuleRef module, struct instruction *insn)
+static LLVMTypeRef insn_symbol_type(struct instruction *insn)
 {
 	if (insn->type)
-		return symbol_type(module, insn->type);
+		return symbol_type(insn->type);
 
 	switch (insn->size) {
 		case 8:		return LLVMInt8Type();
@@ -310,7 +310,7 @@ static LLVMValueRef val_to_value(struct function *fn, unsigned long long val, st
 	LLVMValueRef result;
 
 	assert(ctype);
-	dtype = symbol_type(fn->module, ctype);
+	dtype = symbol_type(ctype);
 	switch (LLVMGetTypeKind(dtype)) {
 	case LLVMPointerTypeKind:
 		itype = LLVMIntType(bits_in_pointer);
@@ -369,7 +369,7 @@ static LLVMValueRef pseudo_to_value(struct function *fn, struct instruction *ins
 			}
 		} else {
 			const char *name = show_ident(sym->ident);
-			LLVMTypeRef type = symbol_type(fn->module, sym);
+			LLVMTypeRef type = symbol_type(sym);
 
 			if (LLVMGetTypeKind(type) == LLVMFunctionTypeKind) {
 				result = LLVMGetNamedFunction(fn->module, name);
@@ -547,7 +547,7 @@ static void output_op_binary(struct function *fn, struct instruction *insn)
 		rhs_nz = LLVMBuildIsNotNull(fn->builder, rhs, "");
 		target = LLVMBuildAnd(fn->builder, lhs_nz, rhs_nz, target_name);
 
-		dst_type = insn_symbol_type(fn->module, insn);
+		dst_type = insn_symbol_type(insn);
 		target = LLVMBuildZExt(fn->builder, target, dst_type, target_name);
 		break;
 	}
@@ -559,7 +559,7 @@ static void output_op_binary(struct function *fn, struct instruction *insn)
 		rhs_nz = LLVMBuildIsNotNull(fn->builder, rhs, "");
 		target = LLVMBuildOr(fn->builder, lhs_nz, rhs_nz, target_name);
 
-		dst_type = insn_symbol_type(fn->module, insn);
+		dst_type = insn_symbol_type(insn);
 		target = LLVMBuildZExt(fn->builder, target, dst_type, target_name);
 		break;
 	}
@@ -585,7 +585,7 @@ static void output_op_compare(struct function *fn, struct instruction *insn)
 
 	pseudo_name(insn->target, target_name);
 
-	LLVMTypeRef dst_type = insn_symbol_type(fn->module, insn);
+	LLVMTypeRef dst_type = insn_symbol_type(insn);
 
 	if (LLVMGetTypeKind(LLVMTypeOf(lhs)) == LLVMIntegerTypeKind) {
 		LLVMIntPredicate op = translate_op(insn->opcode);
@@ -627,7 +627,7 @@ static LLVMValueRef calc_memop_addr(struct function *fn, struct instruction *ins
 	/* convert src to the effective pointer type */
 	src = pseudo_to_value(fn, insn, insn->src);
 	as = LLVMGetPointerAddressSpace(LLVMTypeOf(src));
-	addr_type = LLVMPointerType(insn_symbol_type(fn->module, insn), as);
+	addr_type = LLVMPointerType(insn_symbol_type(insn), as);
 	src = LLVMBuildPointerCast(fn->builder, src, addr_type, "");
 
 	/* addr = src + off */
@@ -812,7 +812,7 @@ static void output_op_ptrcast(struct function *fn, struct instruction *insn)
 
 	assert(!symbol_is_fp_type(insn->type));
 
-	dtype = insn_symbol_type(fn->module, insn);
+	dtype = insn_symbol_type(insn);
 	switch (LLVMGetTypeKind(LLVMTypeOf(src))) {
 	case LLVMPointerTypeKind:
 		op = LLVMBitCast;
@@ -846,7 +846,7 @@ static void output_op_cast(struct function *fn, struct instruction *insn, LLVMOp
 
 	assert(!symbol_is_fp_type(insn->type));
 
-	dtype = insn_symbol_type(fn->module, insn);
+	dtype = insn_symbol_type(insn);
 	switch (LLVMGetTypeKind(LLVMTypeOf(src))) {
 	case LLVMPointerTypeKind:
 		op = LLVMPtrToInt;
@@ -1023,12 +1023,12 @@ static void output_fn(LLVMModuleRef module, struct entrypoint *ep)
 	FOR_EACH_PTR(base_type->arguments, arg) {
 		struct symbol *arg_base_type = arg->ctype.base_type;
 
-		arg_types[nr_args++] = symbol_type(module, arg_base_type);
+		arg_types[nr_args++] = symbol_type(arg_base_type);
 	} END_FOR_EACH_PTR(arg);
 
 	name = show_ident(sym->ident);
 
-	return_type = symbol_type(module, ret_type);
+	return_type = symbol_type(ret_type);
 
 	function.type = LLVMFunctionType(return_type, arg_types, nr_args, 0);
 
@@ -1065,7 +1065,7 @@ static void output_fn(LLVMModuleRef module, struct entrypoint *ep)
 			/* insert alloca into entry block */
 			entrybbr = LLVMGetEntryBasicBlock(function.fn);
 			LLVMPositionBuilderAtEnd(function.builder, entrybbr);
-			phi_type = insn_symbol_type(module, insn);
+			phi_type = insn_symbol_type(insn);
 			ptr = LLVMBuildAlloca(function.builder, phi_type, "");
 			/* emit forward load for phi */
 			LLVMClearInsertionPosition(function.builder);
@@ -1095,7 +1095,7 @@ static LLVMValueRef output_data(LLVMModuleRef module, struct symbol *sym)
 	if (initializer) {
 		switch (initializer->type) {
 		case EXPR_VALUE:
-			initial_value = LLVMConstInt(symbol_type(module, sym), initializer->value, 1);
+			initial_value = LLVMConstInt(symbol_type(sym), initializer->value, 1);
 			break;
 		case EXPR_SYMBOL: {
 			struct symbol *sym = initializer->symbol;
@@ -1115,7 +1115,7 @@ static LLVMValueRef output_data(LLVMModuleRef module, struct symbol *sym)
 			assert(0);
 		}
 	} else {
-		LLVMTypeRef type = symbol_type(module, sym);
+		LLVMTypeRef type = symbol_type(sym);
 
 		initial_value = LLVMConstNull(type);
 	}
-- 
2.11.1

[PATCH 12/13] llvm: remove unneeded arg 'fn'

[Luc Van Oostenryck]

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 sparse-llvm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sparse-llvm.c b/sparse-llvm.c
index 35562e570..7c7b70027 100644
--- a/sparse-llvm.c
+++ b/sparse-llvm.c
@@ -303,7 +303,7 @@ static void pseudo_name(pseudo_t pseudo, char *buf)
 	}
 }
 
-static LLVMValueRef val_to_value(struct function *fn, unsigned long long val, struct symbol *ctype)
+static LLVMValueRef val_to_value(unsigned long long val, struct symbol *ctype)
 {
 	LLVMTypeRef dtype;
 	LLVMTypeRef itype;
@@ -384,7 +384,7 @@ static LLVMValueRef pseudo_to_value(struct function *fn, struct instruction *ins
 		break;
 	}
 	case PSEUDO_VAL:
-		result = val_to_value(fn, pseudo->value, insn->type);
+		result = val_to_value(pseudo->value, insn->type);
 		break;
 	case PSEUDO_ARG: {
 		result = LLVMGetParam(fn->fn, pseudo->nr - 1);
@@ -749,7 +749,7 @@ static void output_op_call(struct function *fn, struct instruction *insn)
 			/* Value pseudos do not have type information. */
 			/* Use the function prototype to get the type. */
 			struct symbol *ctype = get_nth1_arg(insn->func->sym, i + 1);
-			value = val_to_value(fn, arg->value, ctype);
+			value = val_to_value(arg->value, ctype);
 		} else {
 			value = pseudo_to_value(fn, insn, arg);
 		}
-- 
2.11.1

[PATCH 13/13] llvm: fix: do not mix pointers and floats when doing compares

[Luc Van Oostenryck]

In output_op_compare() everything that is not of interger
type is treated as floats, pointers disagree.

Fix this by rearranging the code and use a switch to easily
treat pointers like integers as required for LLVM's icmp.

Reported-by: Dibyendu Majumdar <mobile@majumdar.org.uk>
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 sparse-llvm.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/sparse-llvm.c b/sparse-llvm.c
index 7c7b70027..be47c5e75 100644
--- a/sparse-llvm.c
+++ b/sparse-llvm.c
@@ -587,14 +587,27 @@ static void output_op_compare(struct function *fn, struct instruction *insn)
 
 	LLVMTypeRef dst_type = insn_symbol_type(insn);
 
-	if (LLVMGetTypeKind(LLVMTypeOf(lhs)) == LLVMIntegerTypeKind) {
+	switch  (LLVMGetTypeKind(LLVMTypeOf(lhs))) {
+	case LLVMPointerTypeKind:
+	case LLVMIntegerTypeKind: {
 		LLVMIntPredicate op = translate_op(insn->opcode);
 
 		target = LLVMBuildICmp(fn->builder, op, lhs, rhs, target_name);
-	} else {
+		break;
+	}
+	case LLVMHalfTypeKind:
+	case LLVMFloatTypeKind:
+	case LLVMDoubleTypeKind:
+	case LLVMX86_FP80TypeKind:
+	case LLVMFP128TypeKind:
+	case LLVMPPC_FP128TypeKind: {
 		LLVMRealPredicate op = translate_fop(insn->opcode);
 
 		target = LLVMBuildFCmp(fn->builder, op, lhs, rhs, target_name);
+		break;
+	}
+	default:
+		assert(0);
 	}
 
 	target = LLVMBuildZExt(fn->builder, target, dst_type, target_name);
-- 
2.11.1

Re: [WIP 00/13] LLVM fixes

[Christopher Li]

Adding CC to Pekka Enberg, he submit most of the llvm patches.

Chris

On Sun, Mar 5, 2017 at 7:20 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> This series solves a number of issues in sparse-llvm,
> mainly about wrong type information as needed for LLVM
> or simply the lack of these infos.
> These issues have been reported and investigated by
> Dibyendu Majumdar.
>
> These patches have been lightly tested and already
> allow to compile a lot more code to LLVM.
> It's still incomplete though:
> - it won't work on bitfields
> - it won't work on computed gotos
> - it won't work on label-as-value
> - it won't work on exotic instructions (OP_SPLICE)
> - few things are working correctly with floats
>   (but this is not specific to sparse-llvm).
> There is most probably a bunch of others issues too.
>
> For testing purpose this serie is also available at:
>   https://github.com/lucvoo/sparse/tree/llvm-fixes-v0
>
>
> Luc Van Oostenryck (13):
>   llvm: add a helper to convert an integer to a ValueRef
>   llvm: fix translation of PSEUDO_VALs into a ValueRefs
>   llvm: fix output_op_store() which modify its operand
>   llvm: fix output_op_[ptr]cast()
>   add get_nth1_arg()
>   llvm: fix type of literal integer passed as arguments
>   llvm: fix output OP_ADD mixed with pointers
>   llvm: add support for OP_NEG
>   give a type to OP_PHISOURCE
>   give a type to OP_SEL, always
>   llvm: remove unneeded arg 'module'
>   llvm: remove unneeded arg 'fn'
>   llvm: fix: do not mix pointers and floats when doing compares
>
>  flow.c                        |   2 +-
>  linearize.c                   |  18 ++--
>  linearize.h                   |   2 +-
>  memops.c                      |   2 +-
>  sparse-llvm.c                 | 187 +++++++++++++++++++++++++++++++-----------
>  symbol.h                      |  13 +++
>  validation/backend/null.c     |  24 ++++++
>  validation/backend/store-x2.c |  16 ++++
>  8 files changed, 203 insertions(+), 61 deletions(-)
>  create mode 100644 validation/backend/null.c
>  create mode 100644 validation/backend/store-x2.c
>
> --
> 2.11.1
>
> --
> 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 05/13] add get_nth1_arg()

[Christopher Li]

On Sun, Mar 5, 2017 at 7:20 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> A backend may need to know the size or the type of an
> argument and there is no easy way to access to this info.

Actually, there is a way to do it with two list iterative. Will comment
it on the next patch that use the get_nth1_arg().


> +static inline struct symbol *get_nth1_arg(struct symbol *fn, int idx)
> +{
> +       struct symbol_list *args = fn->ctype.base_type->arguments;
> +       struct symbol *arg;
> +       int i = 0;
> +       FOR_EACH_PTR(args, arg) {
> +               if (++i == idx)
> +                       return arg;
> +       } END_FOR_EACH_PTR(arg);
> +
> +       return NULL;

As I said, I am not sure this is necessary in the particular case.
However, the nth list entry is some what useful. If we add that,
we should add it to ptrlist.c. Then every time of ptr_list can have it.

Also, there is no need to do "++i", it should use the list->nr to skip
over the list node for the case where n is relative large. Then only do the
counting inside the list node. If list is packed, it is every easier, we can
just return from the index. Depend on we want to allow unpacked list
or not.

Chris

Re: [PATCH 06/13] llvm: fix type of literal integer passed as arguments

[Christopher Li]

On Sun, Mar 5, 2017 at 7:20 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> Like for all others instructions, LLVM needs the type
> of each operands. However this information is not always
> available via the pseudo, like here when passing a integer
> constant as argument since for sparse constants are typeless.
>
> Fix this by getting the type via the function prototype.
>
> Reported-by: Dibyendu Majumdar <mobile@majumdar.org.uk>
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>  sparse-llvm.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/sparse-llvm.c b/sparse-llvm.c
> index c593f831f..27cc1b88c 100644
> --- a/sparse-llvm.c
> +++ b/sparse-llvm.c
> @@ -740,7 +740,16 @@ static void output_op_call(struct function *fn, struct instruction *insn)

Adding CC to Jeff Garzik,

I notice a near by line in the same function does:

FOR_EACH_PTR(insn->arguments, arg) {
                  n_arg++;
} END_FOR_EACH_PTR(arg);

Shouldn't it just use ptr_list_size() instead?

>
>         i = 0;
>         FOR_EACH_PTR(insn->arguments, arg) {
> -               args[i++] = pseudo_to_value(fn, insn, arg);
> +               LLVMValueRef value;
> +               if (arg->type == PSEUDO_VAL) {
> +                       /* Value pseudos do not have type information. */
> +                       /* Use the function prototype to get the type. */
> +                       struct symbol *ctype = get_nth1_arg(insn->func->sym, i + 1);

Here is an usage case of iterate through two separate list in order.
You can use PREPARE_PTR_LIST() and NEXT_PTR_LIST()
with FOR_EACH_PTR() to iterate the ins->func->sym list in order.

You can take a look at evaluate.c how to use PREPARE_PTR_LIST
with FOR_EACH_PTR().  e.g. evaluate.c::evaluate_arguments().

If we use get_nth1_arg here. We are effectively using two for loops.
The outer loop count from one to nth argument. The inner loop in
get_nth1_arg() count from one to nth argument again.

Using PREPARE_PTR_LIST(), it has just one loop.

Chris

Re: [PATCH 07/13] llvm: fix output OP_ADD mixed with pointers

[Christopher Li]

On Sun, Mar 5, 2017 at 7:20 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> In sparse, pointer arithmetic and accessing the field
> of a structure or an array is simply done via OP_ADD,
> the offset being calculated at evaluation time.
> On the other hand, LLVM allows addition only on two
> integers and pointer arithmetic/member access is done
> via 'getelementptr'.
>
> sparse-llvm didn't took this in account which resulted
> in type error in 'add' instructions.
>
> Fix this by catching addition involving pointer and issuing
> a getelementptr' instruction for these.

I have one related question. In the case of anonymous
structure or union, how to you figure out which series of
GEP it needs to be? I think sparse already lost the element
pointer index information. You can construct it back by looking
at the bit offset. But if there is union then the element point can
have multiple path to reach to the same bit offset. I don't
know how to deal with that.

>
> Originally-by: Dibyendu Majumdar <mobile@majumdar.org.uk>
> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> ---
>  sparse-llvm.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/sparse-llvm.c b/sparse-llvm.c
> index 27cc1b88c..ee374b217 100644
> --- a/sparse-llvm.c
> +++ b/sparse-llvm.c
> @@ -472,6 +472,10 @@ static void output_op_binary(struct function *fn, struct instruction *insn)
>         case OP_ADD:
>                 if (symbol_is_fp_type(insn->type))
>                         target = LLVMBuildFAdd(fn->builder, lhs, rhs, target_name);
> +               else if (LLVMGetTypeKind(LLVMTypeOf(lhs)) == LLVMPointerTypeKind)
> +                       target = LLVMBuildGEP(fn->builder, lhs, &rhs, 1, "");
> +               else if (LLVMGetTypeKind(LLVMTypeOf(rhs)) == LLVMPointerTypeKind)
> +                       target = LLVMBuildGEP(fn->builder, rhs, &lhs, 1, "");

It seems that you only have one level of indices. Also the OP_ADD use the member
offset of the struct. I am not sure how it map into GEP indices.
Correct me if I am
wrong, it seems to me you use the member offset as element indices. I
think it need
to get a mapping between offset to indices.

Chris

Re: [PATCH 07/13] llvm: fix output OP_ADD mixed with pointers

[Dibyendu Majumdar]

Hi Chris,

On 6 March 2017 at 15:16, Christopher Li <sparse@chrisli.org> wrote:
> On Sun, Mar 5, 2017 at 7:20 PM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
>> In sparse, pointer arithmetic and accessing the field
>> of a structure or an array is simply done via OP_ADD,
>> the offset being calculated at evaluation time.
>> On the other hand, LLVM allows addition only on two
>> integers and pointer arithmetic/member access is done
>> via 'getelementptr'.
>>
>> sparse-llvm didn't took this in account which resulted
>> in type error in 'add' instructions.
>>
>> Fix this by catching addition involving pointer and issuing
>> a getelementptr' instruction for these.
>
> I have one related question. In the case of anonymous
> structure or union, how to you figure out which series of
> GEP it needs to be? I think sparse already lost the element
> pointer index information. You can construct it back by looking
> at the bit offset. But if there is union then the element point can
> have multiple path to reach to the same bit offset. I don't
> know how to deal with that.

Sparse-llvm appears to bypass the normal struct GEP in LLVM. It
basically casts everything to char *, uses GEP to obtain a pointer to
member, and then casts it back to member type. So this should work for
structs and unions.

>
>>
>> Originally-by: Dibyendu Majumdar <mobile@majumdar.org.uk>
>> Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
>> ---
>>  sparse-llvm.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/sparse-llvm.c b/sparse-llvm.c
>> index 27cc1b88c..ee374b217 100644
>> --- a/sparse-llvm.c
>> +++ b/sparse-llvm.c
>> @@ -472,6 +472,10 @@ static void output_op_binary(struct function *fn, struct instruction *insn)
>>         case OP_ADD:
>>                 if (symbol_is_fp_type(insn->type))
>>                         target = LLVMBuildFAdd(fn->builder, lhs, rhs, target_name);
>> +               else if (LLVMGetTypeKind(LLVMTypeOf(lhs)) == LLVMPointerTypeKind)
>> +                       target = LLVMBuildGEP(fn->builder, lhs, &rhs, 1, "");
>> +               else if (LLVMGetTypeKind(LLVMTypeOf(rhs)) == LLVMPointerTypeKind)
>> +                       target = LLVMBuildGEP(fn->builder, rhs, &lhs, 1, "");
>
> It seems that you only have one level of indices. Also the OP_ADD use the member
> offset of the struct. I am not sure how it map into GEP indices.
> Correct me if I am
> wrong, it seems to me you use the member offset as element indices. I
> think it need
> to get a mapping between offset to indices.
>

This case is more about handing pointer arithmetic correctly. Here GEP
is being used as array element access and not for struct member
access.

Hope this is helpful.

Regards
Dibyendu

Re: [PATCH 07/13] llvm: fix output OP_ADD mixed with pointers

[Christopher Li]

On Mon, Mar 6, 2017 at 11:32 PM, Dibyendu Majumdar
<mobile@majumdar.org.uk> wrote:
>
> Sparse-llvm appears to bypass the normal struct GEP in LLVM. It
> basically casts everything to char *, uses GEP to obtain a pointer to
> member, and then casts it back to member type. So this should work for
> structs and unions.

That is exactly my question. I haven't understand how this by pass
can work in all the situation.

>
> This case is more about handing pointer arithmetic correctly. Here GEP
> is being used as array element access and not for struct member
> access.

Even using array element access, I still have some question regarding
the the mapping process of the array index.
Correct me if I am wrong, the GEP indices are express as array index.
So after convert to (char*) + offset, the offset will express as
n*(sizeof(element type)).
"n" is the array index.

However, the OP_ADD instruction, the offset is byte offset value. The offset
part might not be align to n*(sizeof(array_elelment)). Assume we can use
some pack struct to get some member located not at the natural alignment offset.

Even if the offset is at the type size alignment, shouldn't the indices express
as ( offset / sizeof(element type))? What do I miss?

Chris

Re: [PATCH 07/13] llvm: fix output OP_ADD mixed with pointers

[Luc Van Oostenryck]

On Tue, Mar 07, 2017 at 12:22:07AM +0800, Christopher Li wrote:
> On Mon, Mar 6, 2017 at 11:32 PM, Dibyendu Majumdar
> <mobile@majumdar.org.uk> wrote:
> >
> > Sparse-llvm appears to bypass the normal struct GEP in LLVM. It
> > basically casts everything to char *, uses GEP to obtain a pointer to
> > member, and then casts it back to member type. So this should work for
> > structs and unions.
> 
> That is exactly my question. I haven't understand how this by pass
> can work in all the situation.
> 
> >
> > This case is more about handing pointer arithmetic correctly. Here GEP
> > is being used as array element access and not for struct member
> > access.
> 
> Even using array element access, I still have some question regarding
> the the mapping process of the array index.
> Correct me if I am wrong, the GEP indices are express as array index.
> So after convert to (char*) + offset, the offset will express as
> n*(sizeof(element type)).
> "n" is the array index.
> 
> However, the OP_ADD instruction, the offset is byte offset value. The offset
> part might not be align to n*(sizeof(array_elelment)). Assume we can use
> some pack struct to get some member located not at the natural alignment offset.
> 
> Even if the offset is at the type size alignment, shouldn't the indices express
> as ( offset / sizeof(element type))? What do I miss?

With an example:
== C code ==
	void *foo(int *p) { return p + 5; }

== linearized code ==
	foo:
	.L0:
	        <entry-point>
	        add.64      %r2 <- %arg1, $20
	        cast.64     %r3 <- (64) %r2
	        ret.64      %r3

== LLVM code from sparse-llvm ==
	; ModuleID = '<stdin>'
	source_filename = "sparse"
	
	define i8* @foo(i32* %ARG1) {
	L0:
	  %0 = getelementptr i32, i32* %ARG1, inttoptr (i64 20 to i32*)
	  %R3 = bitcast i32* %0 to i8*
	  ret i8* %R3
	}


-- Luc

Re: [PATCH 05/13] add get_nth1_arg()

[Luc Van Oostenryck]

On Mon, Mar 06, 2017 at 10:40:17PM +0800, Christopher Li wrote:
> On Sun, Mar 5, 2017 at 7:20 PM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> > A backend may need to know the size or the type of an
> > argument and there is no easy way to access to this info.
> 
> Actually, there is a way to do it with two list iterative. Will comment
> it on the next patch that use the get_nth1_arg().
> 
> 
> > +static inline struct symbol *get_nth1_arg(struct symbol *fn, int idx)
> > +{
> > +       struct symbol_list *args = fn->ctype.base_type->arguments;
> > +       struct symbol *arg;
> > +       int i = 0;
> > +       FOR_EACH_PTR(args, arg) {
> > +               if (++i == idx)
> > +                       return arg;
> > +       } END_FOR_EACH_PTR(arg);
> > +
> > +       return NULL;
> 
> As I said, I am not sure this is necessary in the particular case.
> However, the nth list entry is some what useful. If we add that,
> we should add it to ptrlist.c. Then every time of ptr_list can have it.
> 
> Also, there is no need to do "++i", it should use the list->nr to skip
> over the list node for the case where n is relative large. Then only do the
> counting inside the list node. If list is packed, it is every easier, we can
> just return from the index. Depend on we want to allow unpacked list
> or not.

Absolutely, this can be optimized.

But this serie is a work in progress to help Dibyendu and I won't
otherwise spend much time on sparse-llvm.

It should also be noted that, in general, counting the list entries
via the list->nr can be problematic as some part of the code store
NULL pointers to remove entries (but it's not the case here with
the arguments).

Luc Van Oostenryck

Re: [PATCH 07/13] llvm: fix output OP_ADD mixed with pointers

[Dibyendu Majumdar]

Hi Luc,

On 6 March 2017 at 16:43, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> On Tue, Mar 07, 2017 at 12:22:07AM +0800, Christopher Li wrote:
>> On Mon, Mar 6, 2017 at 11:32 PM, Dibyendu Majumdar
>> <mobile@majumdar.org.uk> wrote:
>> >
>> > Sparse-llvm appears to bypass the normal struct GEP in LLVM. It
>> > basically casts everything to char *, uses GEP to obtain a pointer to
>> > member, and then casts it back to member type. So this should work for
>> > structs and unions.
>>
>> That is exactly my question. I haven't understand how this by pass
>> can work in all the situation.
>>
>> >
>> > This case is more about handing pointer arithmetic correctly. Here GEP
>> > is being used as array element access and not for struct member
>> > access.
>>
>> Even using array element access, I still have some question regarding
>> the the mapping process of the array index.
>> Correct me if I am wrong, the GEP indices are express as array index.
>> So after convert to (char*) + offset, the offset will express as
>> n*(sizeof(element type)).
>> "n" is the array index.
>>
>> However, the OP_ADD instruction, the offset is byte offset value. The offset
>> part might not be align to n*(sizeof(array_elelment)). Assume we can use
>> some pack struct to get some member located not at the natural alignment offset.
>>
>> Even if the offset is at the type size alignment, shouldn't the indices express
>> as ( offset / sizeof(element type))? What do I miss?
>
> With an example:
> == C code ==
>         void *foo(int *p) { return p + 5; }
>
> == linearized code ==
>         foo:
>         .L0:
>                 <entry-point>
>                 add.64      %r2 <- %arg1, $20
>                 cast.64     %r3 <- (64) %r2
>                 ret.64      %r3
>
> == LLVM code from sparse-llvm ==
>         ; ModuleID = '<stdin>'
>         source_filename = "sparse"
>
>         define i8* @foo(i32* %ARG1) {
>         L0:
>           %0 = getelementptr i32, i32* %ARG1, inttoptr (i64 20 to i32*)
>           %R3 = bitcast i32* %0 to i8*
>           ret i8* %R3
>         }
>

This may be the issue Chris is highlighting -  above the offset should
be 5 not 20 in the LLVM code as the array type is i32* not i8*. Does
sparse always output array offsets in char*?

Regards
Dibyendu

Re: [PATCH 07/13] llvm: fix output OP_ADD mixed with pointers

[Christopher Li]

On Tue, Mar 7, 2017 at 12:43 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> With an example:
> == C code ==
>         void *foo(int *p) { return p + 5; }
>
> == linearized code ==
>         foo:
>         .L0:
>                 <entry-point>
>                 add.64      %r2 <- %arg1, $20
>                 cast.64     %r3 <- (64) %r2
>                 ret.64      %r3
>
> == LLVM code from sparse-llvm ==
>         ; ModuleID = '<stdin>'
>         source_filename = "sparse"
>
>         define i8* @foo(i32* %ARG1) {
>         L0:
>           %0 = getelementptr i32, i32* %ARG1, inttoptr (i64 20 to i32*)
>           %R3 = bitcast i32* %0 to i8*
>           ret i8* %R3
>         }
>


OK, good. Let's use this example.
I am using clang to get the llvm bytecode. t.c is your example.

clang -S -emit-llvm /tmp/t.c
Here is output, compare the line I am pointing at:
The indices should be 5 according clang's output.
However sparse-llvm generate as 20 per your output
if I am reading it correctly.

Chris

============== t.ll =====================
 ModuleID = '/tmp/t.c'
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

; Function Attrs: nounwind uwtable
define i8* @foo(i32* %p) #0 {
  %1 = alloca i32*, align 8
  store i32* %p, i32** %1, align 8
  %2 = load i32*, i32** %1, align 8
  %3 = getelementptr inbounds i32, i32* %2, i64 5 <======== look here
  %4 = bitcast i32* %3 to i8*
  ret i8* %4
}

attributes #0 = { nounwind uwtable "disable-tail-calls"="false"
"less-precise-fpmad"="false" "no-frame-pointer-elim"="true"
"no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false"
"no-nans-fp-math"="false" "stack-protector-buffer-size"="8"
"target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2"
"unsafe-fp-math"="false" "use-soft-float"="false" }

!llvm.ident = !{!0}

!0 = !{!"clang version 3.8.1 (tags/RELEASE_381/final)"}
================================================

Re: [PATCH 07/13] llvm: fix output OP_ADD mixed with pointers

[Linus Torvalds]

On Mon, Mar 6, 2017 at 8:43 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> With an example:
> == C code ==
>         void *foo(int *p) { return p + 5; }
>
> == linearized code ==
>         foo:
>         .L0:
>                 <entry-point>
>                 add.64      %r2 <- %arg1, $20
>                 cast.64     %r3 <- (64) %r2
>                 ret.64      %r3

This is correct.

> == LLVM code from sparse-llvm ==
>
>         define i8* @foo(i32* %ARG1) {
>         L0:
>           %0 = getelementptr i32, i32* %ARG1, inttoptr (i64 20 to i32*)
>           %R3 = bitcast i32* %0 to i8*

This is garbage, I'm afraid.

When sparse does the "add 20 to pointer", it adds the *byte offset* 20
to the pointer. The LLVM module should not use "getelementptr" for
this, because it's not element #20, it's the element at offset 20.

I think you're supposed to either use "uglygep" with the base pointer
cast to a simple address-unit pointer (ie unsigned char).

Or not use GEP at all.

                Linus

Re: [PATCH 07/13] llvm: fix output OP_ADD mixed with pointers

[Luc Van Oostenryck]

On Mon, Mar 06, 2017 at 05:06:01PM +0000, Dibyendu Majumdar wrote:
> 
> This may be the issue Chris is highlighting -  above the offset should
> be 5 not 20 in the LLVM code as the array type is i32* not i8*. Does
> sparse always output array offsets in char*?

Yes, of course.

I think that the best way to see things is not to consider that 
"sparse output offset in char*". As you have already seen, what
sparse produce after linearization is more low-level that LLVM's IR,
it's closer to what a CPU would see. So I think you should see
sparse's offset simply as an *address offset*, the values involved
being the values that would in the register of some CPU.

-- Luc 

Re: [PATCH 07/13] llvm: fix output OP_ADD mixed with pointers

[Luc Van Oostenryck]

On Tue, Mar 07, 2017 at 01:07:27AM +0800, Christopher Li wrote:
> On Tue, Mar 7, 2017 at 12:43 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> >
> > With an example:
> > == C code ==
> >         void *foo(int *p) { return p + 5; }
> >
> > == linearized code ==
> >         foo:
> >         .L0:
> >                 <entry-point>
> >                 add.64      %r2 <- %arg1, $20
> >                 cast.64     %r3 <- (64) %r2
> >                 ret.64      %r3
> >
> > == LLVM code from sparse-llvm ==
> >         ; ModuleID = '<stdin>'
> >         source_filename = "sparse"
> >
> >         define i8* @foo(i32* %ARG1) {
> >         L0:
> >           %0 = getelementptr i32, i32* %ARG1, inttoptr (i64 20 to i32*)
> >           %R3 = bitcast i32* %0 to i8*
> >           ret i8* %R3
> >         }
> >
> 
> 
> OK, good. Let's use this example.
> I am using clang to get the llvm bytecode. t.c is your example.
> 
> clang -S -emit-llvm /tmp/t.c
> Here is output, compare the line I am pointing at:
> The indices should be 5 according clang's output.
> However sparse-llvm generate as 20 per your output
> if I am reading it correctly.

Absolutely.

-- Luc 

Re: [PATCH 07/13] llvm: fix output OP_ADD mixed with pointers

[Luc Van Oostenryck]

On Mon, Mar 06, 2017 at 10:17:35AM -0800, Linus Torvalds wrote:
> On Mon, Mar 6, 2017 at 8:43 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> >
> > With an example:
> > == C code ==
> >         void *foo(int *p) { return p + 5; }
> >
> > == linearized code ==
> >         foo:
> >         .L0:
> >                 <entry-point>
> >                 add.64      %r2 <- %arg1, $20
> >                 cast.64     %r3 <- (64) %r2
> >                 ret.64      %r3
> 
> This is correct.

Interestingly, the following code is linearized a bit differently:
== C code ==
	void *foo(int *p) { return p += 5; }
== linearized code ==
	foo:
	.L0:
	        <entry-point>
	        cast.64     %r98 <- (64) %arg1
	        add.64      %r99 <- %r98, $20
	        ptrcast.64  %r100 <- (64) %r99
	        cast.64     %r101 <- (64) %r100
	        ret.64      %r101

Where the type of %r98 & %r99 is 'unsigned long' (or more probably size_t).
This is then correctly LLVMized as:
	define i8* @foo(i32* %ARG1) {
	L18:
	  %R98 = ptrtoint i32* %ARG1 to i64
	  %R99 = add i64 %R98, 20
	  %R100 = inttoptr i64 %R99 to i32*
	  %R101 = bitcast i32* %R100 to i8*
	  ret i8* %R101
	}
 
> > == LLVM code from sparse-llvm ==
> >
> >         define i8* @foo(i32* %ARG1) {
> >         L0:
> >           %0 = getelementptr i32, i32* %ARG1, inttoptr (i64 20 to i32*)
> >           %R3 = bitcast i32* %0 to i8*
> 
> This is garbage, I'm afraid.
> 
> When sparse does the "add 20 to pointer", it adds the *byte offset* 20
> to the pointer. The LLVM module should not use "getelementptr" for
> this, because it's not element #20, it's the element at offset 20.

It's clear that there is a semantic gap between sparse's & LLVM's IR.

> I think you're supposed to either use "uglygep" with the base pointer
> cast to a simple address-unit pointer (ie unsigned char).
> 
> Or not use GEP at all.
> 
>                 Linus

I bet we'll have others problems with GEP.

Luc

[PATCH v2] llvm: fix output OP_ADD mixed with pointers

[Luc Van Oostenryck]

In sparse, pointer arithmetic and accessing the field
of a structure or an array is simply done via OP_ADD,
the offset being calculated at evaluation time.
On the other hand, LLVM allows addition only on two
integers and pointer arithmetic/member access must be
done either via 'getelementptr' or the pointer must be
casted to and fro an integer.

sparse-llvm didn't took this in account which resulted
in type error in 'add' instructions.

Fix this by catching addition involving pointer and
the already existing helper calc_gep() for these.

Originally-by: Dibyendu Majumdar <mobile@majumdar.org.uk>
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---

Changes since v1:
- use calc_gep() to issue the 'getelementptr' in order to use the
  offset value.

 sparse-llvm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sparse-llvm.c b/sparse-llvm.c
index 27cc1b88c..bd57730d4 100644
--- a/sparse-llvm.c
+++ b/sparse-llvm.c
@@ -472,6 +472,10 @@ static void output_op_binary(struct function *fn, struct instruction *insn)
 	case OP_ADD:
 		if (symbol_is_fp_type(insn->type))
 			target = LLVMBuildFAdd(fn->builder, lhs, rhs, target_name);
+		else if (LLVMGetTypeKind(LLVMTypeOf(lhs)) == LLVMPointerTypeKind)
+			target = calc_gep(fn->builder, lhs, rhs);
+		else if (LLVMGetTypeKind(LLVMTypeOf(rhs)) == LLVMPointerTypeKind)
+			target = calc_gep(fn->builder, rhs, lhs);
 		else
 			target = LLVMBuildAdd(fn->builder, lhs, rhs, target_name);
 		break;
-- 
2.11.1

Re: [PATCH 02/13] llvm: fix translation of PSEUDO_VALs into a ValueRefs

[Christopher Li]

On Sun, Mar 5, 2017 at 7:20 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> In sparse-llvm there is the assumption that a PSEUDO_VAL is always
> of integer type. But this is not always the case: constant pointers,
> like NULL, are also of the PSEUDO_VAL kind.

After apply this patch, the test suite has three more new test failure.
A closer look there are assert failed in three of the test cases.

error: actual error text does not match expected error text.
error: see backend/loop2.c.error.* for further investigation.
--- backend/loop2.c.error.expected 2017-03-07 23:06:42.298291232 +0800
+++ backend/loop2.c.error.got 2017-03-07 23:06:42.460291747 +0800
+sparse-llvm: sparse-llvm.c:312: val_to_value: Assertion `ctype' failed.
+.././sparsec: line 35:  8495 Aborted                 (core dumped)
$DIRNAME/sparse-llvm $SPARSEOPTS > $TMPLLVM


Also the previous 1/13 patch will generate warning without this path.

sparse-llvm.c:306:21: warning: ‘val_to_value’ defined but not used
[-Wunused-function]
 static LLVMValueRef val_to_value(struct function *fn, unsigned long
long val, struct symbol *ctype)
                     ^~~~~~~~~~~~
We should fix the warning. Maybe combine the previous patch with this one?

Chris

Re: [PATCH 06/13] llvm: fix type of literal integer passed as arguments

[Christopher Li]

[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]

On Sun, Mar 5, 2017 at 7:20 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> Like for all others instructions, LLVM needs the type
> of each operands. However this information is not always
> available via the pseudo, like here when passing a integer
> constant as argument since for sparse constants are typeless.
>
> Fix this by getting the type via the function prototype.
>
> +               LLVMValueRef value;
> +               if (arg->type == PSEUDO_VAL) {
> +                       /* Value pseudos do not have type information. */
> +                       /* Use the function prototype to get the type. */
> +                       struct symbol *ctype = get_nth1_arg(insn->func->sym, i + 1);

I try to come up with an example to use the PREPARE_PTR_LIST() in this patch.
I hit a bug "insn->func->sym"  assume "insn->func" is a function symbol node.
If "insn->func" is a function pointer then access "insn->func->sym" is wrong.

Any way, my modify patch attached. It should work similar to this patch
without using the nth argument help function. My limited test hit this
function pointer bug.


Chris

[-- Attachment #2: 9604557-06-13-llvm-fix-type-of-literal-integer-passed-as-arguments-1.patch --]
[-- Type: text/x-patch, Size: 2096 bytes --]

From patchwork Sun Mar  5 11:20:40 2017
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: [06/13] llvm: fix type of literal integer passed as arguments
From: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
X-Patchwork-Id: 9604557
Message-Id: <20170305112047.3411-7-luc.vanoostenryck@gmail.com>
To: linux-sparse@vger.kernel.org
Cc: Dibyendu Majumdar <mobile@majumdar.org.uk>,
 Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
Date: Sun,  5 Mar 2017 12:20:40 +0100

Like for all others instructions, LLVM needs the type
of each operands. However this information is not always
available via the pseudo, like here when passing a integer
constant as argument since for sparse constants are typeless.

Fix this by getting the type via the function prototype.

Reported-by: Dibyendu Majumdar <mobile@majumdar.org.uk>
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 sparse-llvm.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)



--- sparse.chrisl.orig/sparse-llvm.c
+++ sparse.chrisl/sparse-llvm.c
@@ -729,6 +729,8 @@ static void output_op_call(struct functi
 {
 	LLVMValueRef target, func;
 	int n_arg = 0, i;
+	struct symbol_list *argument_types = insn->func->sym->ctype.base_type->arguments;
+	struct symbol *argtype;
 	struct pseudo *arg;
 	LLVMValueRef *args;
 
@@ -739,9 +741,20 @@ static void output_op_call(struct functi
 	args = calloc(n_arg, sizeof(LLVMValueRef));
 
 	i = 0;
+	PREPARE_PTR_LIST(argument_types, argtype);
 	FOR_EACH_PTR(insn->arguments, arg) {
-		args[i++] = pseudo_to_value(fn, insn, arg);
+		LLVMValueRef value;
+		if (arg->type == PSEUDO_VAL) {
+			/* Value pseudos do not have type information. */
+			/* Use the function prototype to get the type. */
+			value = val_to_value(fn, arg->value, argtype);
+		} else {
+			value = pseudo_to_value(fn, insn, arg);
+		}
+		args[i++] = value;
+		NEXT_PTR_LIST(argtype);
 	} END_FOR_EACH_PTR(arg);
+	FINISH_PTR_LIST(argtype);
 
 	func = pseudo_to_value(fn, insn, insn->func);
 	target = LLVMBuildCall(fn->builder, func, args, n_arg, "");

Re: [PATCH 02/13] llvm: fix translation of PSEUDO_VALs into a ValueRefs

[Luc Van Oostenryck]

On Tue, Mar 07, 2017 at 11:11:20PM +0800, Christopher Li wrote:
> On Sun, Mar 5, 2017 at 7:20 PM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> > In sparse-llvm there is the assumption that a PSEUDO_VAL is always
> > of integer type. But this is not always the case: constant pointers,
> > like NULL, are also of the PSEUDO_VAL kind.
> 
> After apply this patch, the test suite has three more new test failure.
> A closer look there are assert failed in three of the test cases.
> 
> error: actual error text does not match expected error text.
> error: see backend/loop2.c.error.* for further investigation.
> --- backend/loop2.c.error.expected 2017-03-07 23:06:42.298291232 +0800
> +++ backend/loop2.c.error.got 2017-03-07 23:06:42.460291747 +0800
> +sparse-llvm: sparse-llvm.c:312: val_to_value: Assertion `ctype' failed.
> +.././sparsec: line 35:  8495 Aborted                 (core dumped)
> $DIRNAME/sparse-llvm $SPARSEOPTS > $TMPLLVM

Mmmm, strange but it's very possible that I didn't tested patch by patch.
I'll look at it.

> Also the previous 1/13 patch will generate warning without this path.
> 
> sparse-llvm.c:306:21: warning: ‘val_to_value’ defined but not used
> [-Wunused-function]
>  static LLVMValueRef val_to_value(struct function *fn, unsigned long
> long val, struct symbol *ctype)
>                      ^~~~~~~~~~~~
> We should fix the warning. Maybe combine the previous patch with this one?

Yes, if you prefer so.
Generally I prefer to separate the introduction of new code and its use
but it doesn't really matter much.

-- Luc 

Re: [PATCH 06/13] llvm: fix type of literal integer passed as arguments

[Luc Van Oostenryck]

On Tue, Mar 07, 2017 at 11:33:19PM +0800, Christopher Li wrote:
> On Sun, Mar 5, 2017 at 7:20 PM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> > Like for all others instructions, LLVM needs the type
> > of each operands. However this information is not always
> > available via the pseudo, like here when passing a integer
> > constant as argument since for sparse constants are typeless.
> >
> > Fix this by getting the type via the function prototype.
> >
> > +               LLVMValueRef value;
> > +               if (arg->type == PSEUDO_VAL) {
> > +                       /* Value pseudos do not have type information. */
> > +                       /* Use the function prototype to get the type. */
> > +                       struct symbol *ctype = get_nth1_arg(insn->func->sym, i + 1);
> 
> I try to come up with an example to use the PREPARE_PTR_LIST() in this patch.
> I hit a bug "insn->func->sym"  assume "insn->func" is a function symbol node.
> If "insn->func" is a function pointer then access "insn->func->sym" is wrong.

Mmmm yes, indeed.
 
> Any way, my modify patch attached. It should work similar to this patch
> without using the nth argument help function. My limited test hit this
> function pointer bug.

OK. 

-- Luc

Re: [PATCH 06/13] llvm: fix type of literal integer passed as arguments

[Dibyendu Majumdar]

On 7 March 2017 at 15:33, Christopher Li <sparse@chrisli.org> wrote:
> On Sun, Mar 5, 2017 at 7:20 PM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
>> Like for all others instructions, LLVM needs the type
>> of each operands. However this information is not always
>> available via the pseudo, like here when passing a integer
>> constant as argument since for sparse constants are typeless.
>>
>> Fix this by getting the type via the function prototype.
>>
>> +               LLVMValueRef value;
>> +               if (arg->type == PSEUDO_VAL) {
>> +                       /* Value pseudos do not have type information. */
>> +                       /* Use the function prototype to get the type. */
>> +                       struct symbol *ctype = get_nth1_arg(insn->func->sym, i + 1);
>
> I try to come up with an example to use the PREPARE_PTR_LIST() in this patch.

I suppose that the function prototype may not have the same number of
declared parameters as the actual call arguments - e.g. if the
function is variadic. In that case there may not be corresponding
parameter definition available. I think that in this case default
argument promotion rules will need to be applied for trailing
arguments.

Regards
Dibyendu

Re: [PATCH 02/13] llvm: fix translation of PSEUDO_VALs into a ValueRefs

[Christopher Li]

On Wed, Mar 8, 2017 at 12:18 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>> We should fix the warning. Maybe combine the previous patch with this one?
>
> Yes, if you prefer so.
> Generally I prefer to separate the introduction of new code and its use
> but it doesn't really matter much.

I don't have a problem with separate new code vs using it. No big deal there.

It is the warning I want to get rid of.

Chris

Re: [PATCH 06/13] llvm: fix type of literal integer passed as arguments

[Dibyendu Majumdar]

On 7 March 2017 at 19:41, Dibyendu Majumdar <mobile@majumdar.org.uk> wrote:
> On 7 March 2017 at 15:33, Christopher Li <sparse@chrisli.org> wrote:
>> On Sun, Mar 5, 2017 at 7:20 PM, Luc Van Oostenryck
>> <luc.vanoostenryck@gmail.com> wrote:
>>> Like for all others instructions, LLVM needs the type
>>> of each operands. However this information is not always
>>> available via the pseudo, like here when passing a integer
>>> constant as argument since for sparse constants are typeless.
>>>
>>> Fix this by getting the type via the function prototype.
>>>
>>> +               LLVMValueRef value;
>>> +               if (arg->type == PSEUDO_VAL) {
>>> +                       /* Value pseudos do not have type information. */
>>> +                       /* Use the function prototype to get the type. */
>>> +                       struct symbol *ctype = get_nth1_arg(insn->func->sym, i + 1);
>>
>> I try to come up with an example to use the PREPARE_PTR_LIST() in this patch.
>
> I suppose that the function prototype may not have the same number of
> declared parameters as the actual call arguments - e.g. if the
> function is variadic. In that case there may not be corresponding
> parameter definition available. I think that in this case default
> argument promotion rules will need to be applied for trailing
> arguments.
>

I have also hit a problem when calling a function via function
pointer. In this case the instruction->func appears to be PSEUDO_REG
rather than PSEUDO_SYM, so trying to get the function prototype
doesn't work.

If the call is a via a function pointer then where should one look for
what the argument type should be when a constant is encountered?

Regards
Dibyendu

Re: [PATCH 06/13] llvm: fix type of literal integer passed as arguments

[Luc Van Oostenryck]

On Fri, Mar 10, 2017 at 5:08 PM, Dibyendu Majumdar
<mobile@majumdar.org.uk> wrote:
> I have also hit a problem when calling a function via function
> pointer. In this case the instruction->func appears to be PSEUDO_REG
> rather than PSEUDO_SYM, so trying to get the function prototype
> doesn't work.
>
> If the call is a via a function pointer then where should one look for
> what the argument type should be when a constant is encountered?

I think I have all such cases fixed.
I'll send an updated patch serie later today or tomorrow.

Luc