All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] implement constant-folding in __builtin_bswap*()
@ 2016-11-17  9:55 Johannes Berg
  2016-11-21  2:38 ` Christopher Li
  0 siblings, 1 reply; 24+ messages in thread
From: Johannes Berg @ 2016-11-17  9:55 UTC (permalink / raw)
  To: sparse; +Cc: linux-sparse

Since gcc does this, it's apparently valid to write

 switch (x) {
 case __builtin_bswap16(12):
   break;
 }

but sparse will flag it as an error today.

The constant folding used to be done in the kernel's htons() and
friends, but due to gcc bugs that isn't done anymore since
commit 7322dd755e7d ("byteswap: try to avoid __builtin_constant_p
gcc bug").

To get rid of the sparse errors on every such instance now, just
add constant folding to __builtin_bswap*() in sparse.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 lib.c                               | 35 ++++++++++++++++++++++++++++++++---
 validation/bswap-constant-folding.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 3 deletions(-)
 create mode 100644 validation/bswap-constant-folding.c

diff --git a/lib.c b/lib.c
index d5b56b012a21..aa2af68bf8d2 100644
--- a/lib.c
+++ b/lib.c
@@ -818,9 +818,38 @@ void declare_builtin_functions(void)
 	add_pre_buffer("extern int __builtin_popcountll(unsigned long long);\n");
 
 	/* And byte swaps.. */
-	add_pre_buffer("extern unsigned short __builtin_bswap16(unsigned short);\n");
-	add_pre_buffer("extern unsigned int __builtin_bswap32(unsigned int);\n");
-	add_pre_buffer("extern unsigned long long __builtin_bswap64(unsigned long long);\n");
+	add_pre_buffer("extern unsigned short ____builtin_bswap16(unsigned short);\n");
+	add_pre_buffer("extern unsigned int ____builtin_bswap32(unsigned int);\n");
+	add_pre_buffer("extern unsigned long long ____builtin_bswap64(unsigned long long);\n");
+	add_pre_buffer("#define __sparse_constant_swab16(x) ((unsigned short)("
+		       "	(((unsigned short)(x) & (unsigned short)0x00ffU) << 8) |"
+		       "	(((unsigned short)(x) & (unsigned short)0xff00U) >> 8)))\n");
+	add_pre_buffer("#define __sparse_constant_swab32(x) ((unsigned int)("
+		       "	(((unsigned int)(x) & (unsigned int)0x000000ffUL) << 24) |"
+		       "	(((unsigned int)(x) & (unsigned int)0x0000ff00UL) <<  8) |"
+		       "	(((unsigned int)(x) & (unsigned int)0x00ff0000UL) >>  8) |"
+		       "	(((unsigned int)(x) & (unsigned int)0xff000000UL) >> 24)))\n");
+	add_pre_buffer("#define __sparse_constant_swab64(x) ((unsigned long long)("
+		       "	(((unsigned long long)(x) & (unsigned long long)0x00000000000000ffULL) << 56) |"
+		       "	(((unsigned long long)(x) & (unsigned long long)0x000000000000ff00ULL) << 40) |"
+		       "	(((unsigned long long)(x) & (unsigned long long)0x0000000000ff0000ULL) << 24) |"
+		       "	(((unsigned long long)(x) & (unsigned long long)0x00000000ff000000ULL) <<  8) |"
+		       "	(((unsigned long long)(x) & (unsigned long long)0x000000ff00000000ULL) >>  8) |"
+		       "	(((unsigned long long)(x) & (unsigned long long)0x0000ff0000000000ULL) >> 24) |"
+		       "	(((unsigned long long)(x) & (unsigned long long)0x00ff000000000000ULL) >> 40) |"
+		       "	(((unsigned long long)(x) & (unsigned long long)0xff00000000000000ULL) >> 56)))\n");
+	add_pre_buffer("#define __builtin_bswap16(x)"
+		       "	(__builtin_constant_p((unsigned short)(x)) ?"
+		       "	__sparse_constant_swab16(x) :"
+		       "	____builtin_bswap16(x))\n");
+	add_pre_buffer("#define __builtin_bswap32(x)"
+		       "	(__builtin_constant_p((unsigned int)(x)) ?"
+		       "	__sparse_constant_swab32(x) :"
+		       "	____builtin_bswap32(x))\n");
+	add_pre_buffer("#define __builtin_bswap64(x)"
+		       "	(__builtin_constant_p((unsigned long long)(x)) ?"
+		       "	__sparse_constant_swab64(x) :"
+		       "	____builtin_bswap64(x))\n");
 
 	/* And atomic memory access functions.. */
 	add_pre_buffer("extern int __sync_fetch_and_add(void *, ...);\n");
diff --git a/validation/bswap-constant-folding.c b/validation/bswap-constant-folding.c
new file mode 100644
index 000000000000..c6511fe62041
--- /dev/null
+++ b/validation/bswap-constant-folding.c
@@ -0,0 +1,28 @@
+typedef unsigned short __be16;
+typedef unsigned short __u16;
+typedef unsigned short u16;
+#define __force
+
+#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
+/* the test behaves as though it's always on a little-endian machine */
+#define __cpu_to_be16(x) ((__force __be16)__swab16((x)))
+#define ___htons(x) __cpu_to_be16(x)
+#define htons(x) ___htons(x)
+
+#define ETH_P_IPV6 0x86DD
+
+static u16 protocol;
+
+static void test(void)
+{
+	switch (protocol) {
+	case htons(ETH_P_IPV6):
+		break;
+	}
+}
+
+/*
+ * check-name: constant folding in bswap builtins
+ * check-error-start
+ * check-error-end
+ */
-- 
2.9.3


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] implement constant-folding in __builtin_bswap*()
  2016-11-17  9:55 [PATCH v2] implement constant-folding in __builtin_bswap*() Johannes Berg
@ 2016-11-21  2:38 ` Christopher Li
  2016-11-21  9:45   ` Johannes Berg
  0 siblings, 1 reply; 24+ messages in thread
From: Christopher Li @ 2016-11-21  2:38 UTC (permalink / raw)
  To: Johannes Berg; +Cc: Linux-Sparse

On Thu, Nov 17, 2016 at 5:55 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> Since gcc does this, it's apparently valid to write
>
>  switch (x) {
>  case __builtin_bswap16(12):
>    break;
>  }
>

Applied. Thanks for the patch. That will definite stop a lot of warning.

There is other possible __builtin_xxx() function gcc expected to get a
const result
when the input arguments are constant.  Ideally, we should be able to
mark __builtin_bswap16
as a special constant "pass through" function. So during the
evaluation and constant propagation,
sparse can pass through the native __builtin_xxx() call to get the
const result. In other words,
sparse shouldn't need to implement "__builtin_bswap16()" by itself. It
can just call to the gcc
one to get the result. Then we can treat all other similar
__builtin_xxx function the same way.

That is a separate patch though.

Chris

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] implement constant-folding in __builtin_bswap*()
  2016-11-21  2:38 ` Christopher Li
@ 2016-11-21  9:45   ` Johannes Berg
  2016-11-22 11:13     ` Christopher Li
  0 siblings, 1 reply; 24+ messages in thread
From: Johannes Berg @ 2016-11-21  9:45 UTC (permalink / raw)
  To: Christopher Li; +Cc: Linux-Sparse


> There is other possible __builtin_xxx() function gcc expected to get
> a const result when the input arguments are constant.  Ideally, we
> should be able to mark __builtin_bswap16 as a special constant "pass
> through" function. So during the evaluation and constant propagation,
> sparse can pass through the native __builtin_xxx() call to get the
> const result. In other words, sparse shouldn't need to implement
> "__builtin_bswap16()" by itself. It can just call to the gcc
> one to get the result. Then we can treat all other similar
> __builtin_xxx function the same way.

Yes, that'd make some sense. I have no idea how to pull it off though
:)

johannes

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] implement constant-folding in __builtin_bswap*()
  2016-11-21  9:45   ` Johannes Berg
@ 2016-11-22 11:13     ` Christopher Li
  2016-11-22 11:39       ` Christopher Li
  2016-11-22 13:15       ` Luc Van Oostenryck
  0 siblings, 2 replies; 24+ messages in thread
From: Christopher Li @ 2016-11-22 11:13 UTC (permalink / raw)
  To: Johannes Berg; +Cc: Linux-Sparse

On Mon, Nov 21, 2016 at 5:45 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> Yes, that'd make some sense. I have no idea how to pull it off though

I take a brief look.

It seems that you need to implement a symbol with ".expand" implement as
"expand_bswap", similar to "expand_constant_p".

Later in in "eval_init_table", just add one entry for __builtin_bswap.
eval_init_table[] = {
{ "__builtin_constant_p", &builtin_fn_type, MOD_TOPLEVEL, &constant_p_op },
{ "__builtin_safe_p", &builtin_fn_type, MOD_TOPLEVEL, &safe_p_op },
{ "__builtin_warning", &builtin_fn_type, MOD_TOPLEVEL, &warning_op },
{ "__builtin_expect", &builtin_fn_type, MOD_TOPLEVEL, &expect_op },
{ "__builtin_choose_expr", &builtin_fn_type, MOD_TOPLEVEL, &choose_op },
{ NULL, NULL, 0 }
};

As you can see in "expand_call", the cost of arguments is zero
if all arguments are constant.

The function "expand_symbol_call" even have the comment about expanding
the builtins there.

In side the "expand_bswap", it should abort if cost is not zero.
It means arguments are not constant, this expression can't
expand into a constant value.

Otherwise, expand_bswap should proceed to calculate the return
value of the bswap call and rewrite the call expression into a constant.
Some what similar to the "__builtin_constant_p".

I will see if I can hack up some thing very quick.

Chris

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] implement constant-folding in __builtin_bswap*()
  2016-11-22 11:13     ` Christopher Li
@ 2016-11-22 11:39       ` Christopher Li
  2016-11-22 13:15       ` Luc Van Oostenryck
  1 sibling, 0 replies; 24+ messages in thread
From: Christopher Li @ 2016-11-22 11:39 UTC (permalink / raw)
  To: Johannes Berg; +Cc: Linux-Sparse

On Tue, Nov 22, 2016 at 7:13 PM, Christopher Li <sparse@chrisli.org> wrote:
> On Mon, Nov 21, 2016 at 5:45 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
>> Yes, that'd make some sense. I have no idea how to pull it off though
>
> I take a brief look.
>
> It seems that you need to implement a symbol with ".expand" implement as
> "expand_bswap", similar to "expand_constant_p".

Never mind that, I just find out expand stage happen after the evaluation
stage. The warning happen in the evaluation stage. It will need to make some
plan change to get it right.

Chris

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] implement constant-folding in __builtin_bswap*()
  2016-11-22 11:13     ` Christopher Li
  2016-11-22 11:39       ` Christopher Li
@ 2016-11-22 13:15       ` Luc Van Oostenryck
  2016-11-22 16:32         ` Christopher Li
  1 sibling, 1 reply; 24+ messages in thread
From: Luc Van Oostenryck @ 2016-11-22 13:15 UTC (permalink / raw)
  To: Christopher Li; +Cc: Johannes Berg, Linux-Sparse, Nicolai Stange

On Tue, Nov 22, 2016 at 07:13:11PM +0800, Christopher Li wrote:
> 
> Otherwise, expand_bswap should proceed to calculate the return
> value of the bswap call and rewrite the call expression into a constant.
> Some what similar to the "__builtin_constant_p".
> 
> I will see if I can hack up some thing very quick.
> 
> Chris
> --

I think it would be best to do any change related tos handling
of constant expressions on to of Nicolai Stange's serie:
	http://marc.info/?l=linux-sparse&m=145429372932235

Not only this serie fixes several issues but it handle the
expression constantnes in a much more systematic way.
Also, I wouldn't like that changes in this area make it
more difficult to integrate this serie (which is already waiting
since July or August 2015).

Luc

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] implement constant-folding in __builtin_bswap*()
  2016-11-22 13:15       ` Luc Van Oostenryck
@ 2016-11-22 16:32         ` Christopher Li
  2016-11-22 17:12           ` Luc Van Oostenryck
                             ` (2 more replies)
  0 siblings, 3 replies; 24+ messages in thread
From: Christopher Li @ 2016-11-22 16:32 UTC (permalink / raw)
  To: Luc Van Oostenryck; +Cc: Johannes Berg, Linux-Sparse, Nicolai Stange

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

On Tue, Nov 22, 2016 at 9:15 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>>
>> I will see if I can hack up some thing very quick.

I get it working in the end. Not too far from what I thought.
Patch attach here for review.

>
> I think it would be best to do any change related tos handling
> of constant expressions on to of Nicolai Stange's serie:
>         http://marc.info/?l=linux-sparse&m=145429372932235

Yes, it is on my patch list. That series can still apply clean.
I just check it.

My current strategy is apply other patches which have impact
on the sparse warning first. Otherwise my anxiety level is too
high, make me hard to pick up and hack sparse. That integer
series needs a relative big chunk of time to review it. For me
it is more important to keep on making progress rather than
solve all problem in one blow.

I think I have been catch up with most of the patches. Should be
able to work on that soon.

Chris

[-- Attachment #2: implement-builtin-bswap.patch --]
[-- Type: text/x-patch, Size: 6130 bytes --]

diff --git a/expand.c b/expand.c
index 0f6720c..1ef95ea 100644
--- a/expand.c
+++ b/expand.c
@@ -802,6 +802,44 @@ int expand_safe_p(struct expression *expr, int cost)
 	return 0;
 }
 
+/* The arguments are constant if the cost of all of them is zero */
+int expand_bswap(struct expression *expr, int cost)
+{
+	struct symbol *sym;
+	struct expression_list *args = expr->args;
+	long long input;
+
+	if (cost)
+		return cost;
+
+	sym = expr->fn->ctype;
+	if (expression_list_size(args) != 1) {
+		sparse_error(expr->pos, "not enough arguments for function %s",
+				show_ident(sym->ident));
+		return SIDE_EFFECTS;
+	}
+
+	input = const_expression_value(first_expression(args));
+
+	if (sym->ident == &__builtin_bswap16_ident) {
+		expr->value = __builtin_bswap16(input);
+		expr->ctype = &ushort_ctype;
+	} else if (sym->ident == &__builtin_bswap32_ident) {
+		expr->value = __builtin_bswap32(input);
+		expr->ctype = &uint_ctype;
+	} else if (sym->ident == &__builtin_bswap64_ident) {
+		expr->value = __builtin_bswap64(input);
+		expr->ctype = &ullong_ctype;
+	} else {
+		die("Unexpected __builtin_bswap symbol %s\n", show_ident(sym->ident));
+	}
+
+	expr->type = EXPR_VALUE;
+	expr->taint = 0;
+	return 0;
+}
+
+
 /*
  * expand a call expression with a symbol. This
  * should expand builtins.
diff --git a/ident-list.h b/ident-list.h
index b65b667..a683f6c 100644
--- a/ident-list.h
+++ b/ident-list.h
@@ -35,6 +35,9 @@ IDENT_RESERVED(__sizeof_ptr__);
 IDENT_RESERVED(__builtin_types_compatible_p);
 IDENT_RESERVED(__builtin_offsetof);
 IDENT_RESERVED(__label__);
+IDENT(__builtin_bswap16);
+IDENT(__builtin_bswap32);
+IDENT(__builtin_bswap64);
 
 /* Attribute names */
 IDENT(packed); IDENT(__packed__);
diff --git a/lib.c b/lib.c
index 2660575..7e35049 100644
--- a/lib.c
+++ b/lib.c
@@ -819,40 +819,6 @@ void declare_builtin_functions(void)
 	add_pre_buffer("extern int __builtin_popcountl(unsigned long);\n");
 	add_pre_buffer("extern int __builtin_popcountll(unsigned long long);\n");
 
-	/* And byte swaps.. */
-	add_pre_buffer("extern unsigned short ____builtin_bswap16(unsigned short);\n");
-	add_pre_buffer("extern unsigned int ____builtin_bswap32(unsigned int);\n");
-	add_pre_buffer("extern unsigned long long ____builtin_bswap64(unsigned long long);\n");
-	add_pre_buffer("#define __sparse_constant_swab16(x) ((unsigned short)("
-		       "	(((unsigned short)(x) & (unsigned short)0x00ffU) << 8) |"
-		       "	(((unsigned short)(x) & (unsigned short)0xff00U) >> 8)))\n");
-	add_pre_buffer("#define __sparse_constant_swab32(x) ((unsigned int)("
-		       "	(((unsigned int)(x) & (unsigned int)0x000000ffUL) << 24) |"
-		       "	(((unsigned int)(x) & (unsigned int)0x0000ff00UL) <<  8) |"
-		       "	(((unsigned int)(x) & (unsigned int)0x00ff0000UL) >>  8) |"
-		       "	(((unsigned int)(x) & (unsigned int)0xff000000UL) >> 24)))\n");
-	add_pre_buffer("#define __sparse_constant_swab64(x) ((unsigned long long)("
-		       "	(((unsigned long long)(x) & (unsigned long long)0x00000000000000ffULL) << 56) |"
-		       "	(((unsigned long long)(x) & (unsigned long long)0x000000000000ff00ULL) << 40) |"
-		       "	(((unsigned long long)(x) & (unsigned long long)0x0000000000ff0000ULL) << 24) |"
-		       "	(((unsigned long long)(x) & (unsigned long long)0x00000000ff000000ULL) <<  8) |"
-		       "	(((unsigned long long)(x) & (unsigned long long)0x000000ff00000000ULL) >>  8) |"
-		       "	(((unsigned long long)(x) & (unsigned long long)0x0000ff0000000000ULL) >> 24) |"
-		       "	(((unsigned long long)(x) & (unsigned long long)0x00ff000000000000ULL) >> 40) |"
-		       "	(((unsigned long long)(x) & (unsigned long long)0xff00000000000000ULL) >> 56)))\n");
-	add_pre_buffer("#define __builtin_bswap16(x)"
-		       "	(__builtin_constant_p((unsigned short)(x)) ?"
-		       "	__sparse_constant_swab16(x) :"
-		       "	____builtin_bswap16(x))\n");
-	add_pre_buffer("#define __builtin_bswap32(x)"
-		       "	(__builtin_constant_p((unsigned int)(x)) ?"
-		       "	__sparse_constant_swab32(x) :"
-		       "	____builtin_bswap32(x))\n");
-	add_pre_buffer("#define __builtin_bswap64(x)"
-		       "	(__builtin_constant_p((unsigned long long)(x)) ?"
-		       "	__sparse_constant_swab64(x) :"
-		       "	____builtin_bswap64(x))\n");
-
 	/* And atomic memory access functions.. */
 	add_pre_buffer("extern int __sync_fetch_and_add(void *, ...);\n");
 	add_pre_buffer("extern int __sync_fetch_and_sub(void *, ...);\n");
diff --git a/lib.h b/lib.h
index b778bdc..306ee45 100644
--- a/lib.h
+++ b/lib.h
@@ -200,6 +200,11 @@ static inline struct instruction *first_instruction(struct instruction_list *hea
 	return first_ptr_list((struct ptr_list *)head);
 }
 
+static inline struct expression *first_expression(struct expression_list *head)
+{
+	return first_ptr_list((struct ptr_list *)head);
+}
+
 static inline pseudo_t first_pseudo(struct pseudo_list *head)
 {
 	return first_ptr_list((struct ptr_list *)head);
diff --git a/symbol.c b/symbol.c
index 92a7a62..e57f207 100644
--- a/symbol.c
+++ b/symbol.c
@@ -773,6 +773,11 @@ static struct symbol_op choose_op = {
 	.args = arguments_choose,
 };
 
+static struct symbol_op bswap_op = {
+	.evaluate = evaluate_to_integer,
+	.expand = expand_bswap
+};
+
 /*
  * Builtin functions
  */
@@ -788,6 +793,9 @@ static struct sym_init {
 	{ "__builtin_warning", &builtin_fn_type, MOD_TOPLEVEL, &warning_op },
 	{ "__builtin_expect", &builtin_fn_type, MOD_TOPLEVEL, &expect_op },
 	{ "__builtin_choose_expr", &builtin_fn_type, MOD_TOPLEVEL, &choose_op },
+	{ "__builtin_bswap16", &builtin_fn_type, MOD_TOPLEVEL, &bswap_op },
+	{ "__builtin_bswap32", &builtin_fn_type, MOD_TOPLEVEL, &bswap_op },
+	{ "__builtin_bswap64", &builtin_fn_type, MOD_TOPLEVEL, &bswap_op },
 	{ NULL,		NULL,		0 }
 };
 
diff --git a/symbol.h b/symbol.h
index 9b3f160..48bbfce 100644
--- a/symbol.h
+++ b/symbol.h
@@ -130,6 +130,7 @@ struct symbol_op {
 
 extern int expand_safe_p(struct expression *expr, int cost);
 extern int expand_constant_p(struct expression *expr, int cost);
+extern int expand_bswap(struct expression *expr, int cost);
 
 #define SYM_ATTR_WEAK		0
 #define SYM_ATTR_NORMAL		1

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] implement constant-folding in __builtin_bswap*()
  2016-11-22 16:32         ` Christopher Li
@ 2016-11-22 17:12           ` Luc Van Oostenryck
  2016-11-23  1:23             ` Christopher Li
  2016-11-22 20:16           ` Luc Van Oostenryck
  2016-11-23  6:28           ` Johannes Berg
  2 siblings, 1 reply; 24+ messages in thread
From: Luc Van Oostenryck @ 2016-11-22 17:12 UTC (permalink / raw)
  To: Christopher Li; +Cc: Johannes Berg, Linux-Sparse, Nicolai Stange

On Wed, Nov 23, 2016 at 12:32:52AM +0800, Christopher Li wrote:
> > I think it would be best to do any change related tos handling
> > of constant expressions on to of Nicolai Stange's serie:
> >         http://marc.info/?l=linux-sparse&m=145429372932235
> 
> Yes, it is on my patch list. That series can still apply clean.
> I just check it.
Great.
 
> My current strategy is apply other patches which have impact
> on the sparse warning first. Otherwise my anxiety level is too
> high, make me hard to pick up and hack sparse. That integer
> series needs a relative big chunk of time to review it. For me
> it is more important to keep on making progress rather than
> solve all problem in one blow.
I understand.

> I think I have been catch up with most of the patches.
Yes, just from memory, there is still one from Oleg, about dissect.

> Should be able to work on that soon.
Great.
 

Luc

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] implement constant-folding in __builtin_bswap*()
  2016-11-22 16:32         ` Christopher Li
  2016-11-22 17:12           ` Luc Van Oostenryck
@ 2016-11-22 20:16           ` Luc Van Oostenryck
  2016-11-23  1:25             ` Christopher Li
  2016-11-23  6:28           ` Johannes Berg
  2 siblings, 1 reply; 24+ messages in thread
From: Luc Van Oostenryck @ 2016-11-22 20:16 UTC (permalink / raw)
  To: Christopher Li; +Cc: Johannes Berg, Linux-Sparse, Nicolai Stange

On Wed, Nov 23, 2016 at 12:32:52AM +0800, Christopher Li wrote:
> On Tue, Nov 22, 2016 at 9:15 PM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> >>
> >> I will see if I can hack up some thing very quick.
> 
> I get it working in the end. Not too far from what I thought.
> Patch attach here for review.

Work great here.
 
If needed here is my
Tested-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] implement constant-folding in __builtin_bswap*()
  2016-11-22 17:12           ` Luc Van Oostenryck
@ 2016-11-23  1:23             ` Christopher Li
  0 siblings, 0 replies; 24+ messages in thread
From: Christopher Li @ 2016-11-23  1:23 UTC (permalink / raw)
  To: Luc Van Oostenryck; +Cc: Johannes Berg, Linux-Sparse, Nicolai Stange

On Wed, Nov 23, 2016 at 1:12 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>> I think I have been catch up with most of the patches.
> Yes, just from memory, there is still one from Oleg, about dissect.

Another observation is that, the review order does not change how
much time it need to review the patches.

Here is my current pending list other than the constant integer expression
series.

Chris

8249571-dissect-teach-do_expression-to-handle-EXPR_OFFSETOF.patch
8250741-dissect-teach-do_initializer-to-handle-the-nested-EXPR_IDENTIFIER-s.patch
8258041-v3-sparse-add-support-for-static-assert.patch
8285451-dissect-s-mode_t-usage_t-in-report_member.patch
8335781-FAIL-1-5-parse-Add-comment-to-struct-statement.patch
8335791-FAIL-3-5-linearize-Add-context-symbol-name-when-showing-context-instructions.patch
8335811-FAIL-5-5-test-locks-Add-lock-tester.patch
8335821-FAIL-4-5-parse-Add-acquire-release-requires-and-guarded_by-attributes.patch
8335851-FAIL-2-5-inspect-Show-context-information.patch
8655361-1-2-compiler.h-add-support-for-malloc-attribute.patch
9013811-2-2-validation-Check-C99-for-loop-variables.patch

9428345-RFC-bits_in_bool-should-be-8.patch

9434581-7-8-explicitely-ignore-killing-OP_ENTRY.patch
9434607-3-5-testsuite-report-as-error-tests-known-to-fail-but-which-succeed.patch
9434617-8-8-cleanup-kill_instruction.patch
9434767-1-2-add-missing-PACK_PTR_LIST.patch
9434769-2-2-mark-lists-to-be-repacked-as-dirty.patch
9434773-4-8-fix-killing-OP_CAST-friends.patch
9435027-6-8-fix-killing-OP_COMPUTEDGOTO.patch
9435155-5-8-fix-killing-OP_SELECT.patch
9435157-3-8-fix-killing-OP_PHI-instructions.patch
9435159-1-8-kill-uses-of-replaced-instructions.patch
9435161-2-8-fix-killing-OP_SETVAL-instructions.patch
9435305-0-2-be-more-generous-with-ptrlist-repacking.patch

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] implement constant-folding in __builtin_bswap*()
  2016-11-22 20:16           ` Luc Van Oostenryck
@ 2016-11-23  1:25             ` Christopher Li
  2016-11-23 20:38               ` [PATCH] add test case for builtin bswap with constant args Luc Van Oostenryck
  2016-11-23 20:48               ` [PATCH v2] implement constant-folding in __builtin_bswap*() Luc Van Oostenryck
  0 siblings, 2 replies; 24+ messages in thread
From: Christopher Li @ 2016-11-23  1:25 UTC (permalink / raw)
  To: Luc Van Oostenryck; +Cc: Johannes Berg, Linux-Sparse, Nicolai Stange

On Wed, Nov 23, 2016 at 4:16 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> Work great here.
>
> If needed here is my
> Tested-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>

Great, that is what I am looking for.

Will apply that patch.

Chris

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] implement constant-folding in __builtin_bswap*()
  2016-11-22 16:32         ` Christopher Li
  2016-11-22 17:12           ` Luc Van Oostenryck
  2016-11-22 20:16           ` Luc Van Oostenryck
@ 2016-11-23  6:28           ` Johannes Berg
  2016-11-23 14:30             ` Christopher Li
  2 siblings, 1 reply; 24+ messages in thread
From: Johannes Berg @ 2016-11-23  6:28 UTC (permalink / raw)
  To: Christopher Li, Luc Van Oostenryck; +Cc: Linux-Sparse, Nicolai Stange


> I get it working in the end. Not too far from what I thought.
> Patch attach here for review.

Just a small comment:

> +       if (expression_list_size(args) != 1) {
> +               sparse_error(expr->pos, "not enough arguments for function %s",
> +                               show_ident(sym->ident));

Should that say "invalid number of arguments (expected 1)" or so?

johannes

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] implement constant-folding in __builtin_bswap*()
  2016-11-23  6:28           ` Johannes Berg
@ 2016-11-23 14:30             ` Christopher Li
  0 siblings, 0 replies; 24+ messages in thread
From: Christopher Li @ 2016-11-23 14:30 UTC (permalink / raw)
  To: Johannes Berg; +Cc: Luc Van Oostenryck, Linux-Sparse, Nicolai Stange

On Wed, Nov 23, 2016 at 2:28 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
>
> Just a small comment:
>
>> +       if (expression_list_size(args) != 1) {
>> +               sparse_error(expr->pos, "not enough arguments for function %s",
>> +                               show_ident(sym->ident));
>
> Should that say "invalid number of arguments (expected 1)" or so?
>

Right, that is better.

Chris

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH] add test case for builtin bswap with constant args
  2016-11-23  1:25             ` Christopher Li
@ 2016-11-23 20:38               ` Luc Van Oostenryck
  2016-11-24  0:17                 ` Christopher Li
  2016-11-24  5:30                 ` Christopher Li
  2016-11-23 20:48               ` [PATCH v2] implement constant-folding in __builtin_bswap*() Luc Van Oostenryck
  1 sibling, 2 replies; 24+ messages in thread
From: Luc Van Oostenryck @ 2016-11-23 20:38 UTC (permalink / raw)
  To: linux-sparse; +Cc: Christopher Li, Luc Van Oostenryck

---
 validation/builtin-constant-eval.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 validation/builtin-constant-eval.c

diff --git a/validation/builtin-constant-eval.c b/validation/builtin-constant-eval.c
new file mode 100644
index 00000000..63e586e9
--- /dev/null
+++ b/validation/builtin-constant-eval.c
@@ -0,0 +1,28 @@
+unsigned short bswap16(void);
+unsigned short bswap16(void)
+{
+	return __builtin_bswap16(0x1234);
+}
+
+unsigned int bswap32(void);
+unsigned int bswap32(void)
+{
+	return __builtin_bswap32(0x12345678);
+}
+
+unsigned long long bswap64(void);
+unsigned long long bswap64(void)
+{
+	return __builtin_bswap64(0x123456789abcdef0ULL);
+}
+
+/*
+ * check-name: builtin-eval
+ * check-command: test-linearize $file
+ *
+ * check-output-ignore
+ * check-output-excludes: __builtin_bswap
+ * check-output-contains:ret.16 *.0x3412
+ * check-output-contains:ret.32 *.0x78563412
+ * check-output-contains:ret.64 *.0xf0debc9a78563412
+ */
-- 
2.10.2


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] implement constant-folding in __builtin_bswap*()
  2016-11-23  1:25             ` Christopher Li
  2016-11-23 20:38               ` [PATCH] add test case for builtin bswap with constant args Luc Van Oostenryck
@ 2016-11-23 20:48               ` Luc Van Oostenryck
  2016-11-24  0:56                 ` Christopher Li
  1 sibling, 1 reply; 24+ messages in thread
From: Luc Van Oostenryck @ 2016-11-23 20:48 UTC (permalink / raw)
  To: Christopher Li; +Cc: Johannes Berg, Linux-Sparse, Nicolai Stange

On Wed, Nov 23, 2016 at 09:25:18AM +0800, Christopher Li wrote:
> On Wed, Nov 23, 2016 at 4:16 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> > Work great here.
> >
> > If needed here is my
> > Tested-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
> 

There is a problem though, with non-constant args.
Now the predeclarations of the 3 __builtin_bswap16/32/64
are gone, no more prototype and thus no more typing information.

For example, if you call __builtin_bswap64() with an int argument,
there is no way for the next steps to know the arg must first
be 64 bit extended. Same for the result's type.

Luc

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] add test case for builtin bswap with constant args
  2016-11-23 20:38               ` [PATCH] add test case for builtin bswap with constant args Luc Van Oostenryck
@ 2016-11-24  0:17                 ` Christopher Li
  2016-11-24  5:30                 ` Christopher Li
  1 sibling, 0 replies; 24+ messages in thread
From: Christopher Li @ 2016-11-24  0:17 UTC (permalink / raw)
  To: Luc Van Oostenryck; +Cc: Linux-Sparse

On Thu, Nov 24, 2016 at 4:38 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> ---
>  validation/builtin-constant-eval.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 validation/builtin-constant-eval.c

Looks good to me. Will apply.

Thanks for the patch.

Chris

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] implement constant-folding in __builtin_bswap*()
  2016-11-23 20:48               ` [PATCH v2] implement constant-folding in __builtin_bswap*() Luc Van Oostenryck
@ 2016-11-24  0:56                 ` Christopher Li
  2016-11-24  0:58                   ` Christopher Li
  2016-11-24  1:31                   ` Luc Van Oostenryck
  0 siblings, 2 replies; 24+ messages in thread
From: Christopher Li @ 2016-11-24  0:56 UTC (permalink / raw)
  To: Luc Van Oostenryck; +Cc: Johannes Berg, Linux-Sparse, Nicolai Stange

On Thu, Nov 24, 2016 at 4:48 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> There is a problem though, with non-constant args.
> Now the predeclarations of the 3 __builtin_bswap16/32/64
> are gone, no more prototype and thus no more typing information.
>
> For example, if you call __builtin_bswap64() with an int argument,
> there is no way for the next steps to know the arg must first
> be 64 bit extended. Same for the result's type.

That is a very good point. Need more work on that.

The currently problem if I include the function prototype, the
function prototype symbol will over shadow the symbol contain
swap_op. We need to make sure the function prototype symbol
is the one loading with correct swap_op.

Ideally, all the "__builtin_xxx" related stuff should be move to a
separate builtin.c. Right now the function prototype is in lib.c
The implementation is sprinkle around symbol.c, evaluation.c
and expand.c.

Re-attach the V2 patch as last email I forget to CC sparse
mailing list. This V2 patch still have the missing prototype problem
you describe here.


Chris

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] implement constant-folding in __builtin_bswap*()
  2016-11-24  0:56                 ` Christopher Li
@ 2016-11-24  0:58                   ` Christopher Li
  2016-11-24  1:36                     ` Luc Van Oostenryck
  2016-11-24  1:31                   ` Luc Van Oostenryck
  1 sibling, 1 reply; 24+ messages in thread
From: Christopher Li @ 2016-11-24  0:58 UTC (permalink / raw)
  To: Luc Van Oostenryck; +Cc: Johannes Berg, Linux-Sparse, Nicolai Stange

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

Err, forget to attach. Here is the still buggy V2 patch.

Chris

On Thu, Nov 24, 2016 at 8:56 AM, Christopher Li <sparse@chrisli.org> wrote:
> On Thu, Nov 24, 2016 at 4:48 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
>> There is a problem though, with non-constant args.
>> Now the predeclarations of the 3 __builtin_bswap16/32/64
>> are gone, no more prototype and thus no more typing information.
>>
>> For example, if you call __builtin_bswap64() with an int argument,
>> there is no way for the next steps to know the arg must first
>> be 64 bit extended. Same for the result's type.
>
> That is a very good point. Need more work on that.
>
> The currently problem if I include the function prototype, the
> function prototype symbol will over shadow the symbol contain
> swap_op. We need to make sure the function prototype symbol
> is the one loading with correct swap_op.
>
> Ideally, all the "__builtin_xxx" related stuff should be move to a
> separate builtin.c. Right now the function prototype is in lib.c
> The implementation is sprinkle around symbol.c, evaluation.c
> and expand.c.
>
> Re-attach the V2 patch as last email I forget to CC sparse
> mailing list. This V2 patch still have the missing prototype problem
> you describe here.
>
>
> Chris

[-- Attachment #2: implement-builtin-bswap-v2.patch --]
[-- Type: text/x-patch, Size: 6194 bytes --]

diff --git a/expand.c b/expand.c
index 0f6720c..b9a430b 100644
--- a/expand.c
+++ b/expand.c
@@ -802,6 +802,46 @@ int expand_safe_p(struct expression *expr, int cost)
 	return 0;
 }
 
+/* The arguments are constant if the cost of all of them is zero */
+int expand_bswap(struct expression *expr, int cost)
+{
+	struct symbol *sym;
+	struct expression_list *args = expr->args;
+	long long input;
+	int count;
+
+	if (cost)
+		return cost;
+
+	sym = expr->fn->ctype;
+	count = expression_list_size(args);
+	if (count != 1) {
+		sparse_error(expr->pos, "Invaild number of arguments for function %s (expected 1 got %d)",
+				show_ident(sym->ident), count);
+		return SIDE_EFFECTS;
+	}
+
+	input = const_expression_value(first_expression(args));
+
+	if (sym->ident == &__builtin_bswap16_ident) {
+		expr->value = __builtin_bswap16(input);
+		expr->ctype = &ushort_ctype;
+	} else if (sym->ident == &__builtin_bswap32_ident) {
+		expr->value = __builtin_bswap32(input);
+		expr->ctype = &uint_ctype;
+	} else if (sym->ident == &__builtin_bswap64_ident) {
+		expr->value = __builtin_bswap64(input);
+		expr->ctype = &ullong_ctype;
+	} else {
+		die("Unexpected __builtin_bswap symbol %s\n", show_ident(sym->ident));
+	}
+
+	expr->type = EXPR_VALUE;
+	expr->taint = 0;
+	return 0;
+}
+
+
 /*
  * expand a call expression with a symbol. This
  * should expand builtins.
diff --git a/ident-list.h b/ident-list.h
index b65b667..a683f6c 100644
--- a/ident-list.h
+++ b/ident-list.h
@@ -35,6 +35,9 @@ IDENT_RESERVED(__sizeof_ptr__);
 IDENT_RESERVED(__builtin_types_compatible_p);
 IDENT_RESERVED(__builtin_offsetof);
 IDENT_RESERVED(__label__);
+IDENT(__builtin_bswap16);
+IDENT(__builtin_bswap32);
+IDENT(__builtin_bswap64);
 
 /* Attribute names */
 IDENT(packed); IDENT(__packed__);
diff --git a/lib.c b/lib.c
index 2660575..7e35049 100644
--- a/lib.c
+++ b/lib.c
@@ -819,40 +819,6 @@ void declare_builtin_functions(void)
 	add_pre_buffer("extern int __builtin_popcountl(unsigned long);\n");
 	add_pre_buffer("extern int __builtin_popcountll(unsigned long long);\n");
 
-	/* And byte swaps.. */
-	add_pre_buffer("extern unsigned short ____builtin_bswap16(unsigned short);\n");
-	add_pre_buffer("extern unsigned int ____builtin_bswap32(unsigned int);\n");
-	add_pre_buffer("extern unsigned long long ____builtin_bswap64(unsigned long long);\n");
-	add_pre_buffer("#define __sparse_constant_swab16(x) ((unsigned short)("
-		       "	(((unsigned short)(x) & (unsigned short)0x00ffU) << 8) |"
-		       "	(((unsigned short)(x) & (unsigned short)0xff00U) >> 8)))\n");
-	add_pre_buffer("#define __sparse_constant_swab32(x) ((unsigned int)("
-		       "	(((unsigned int)(x) & (unsigned int)0x000000ffUL) << 24) |"
-		       "	(((unsigned int)(x) & (unsigned int)0x0000ff00UL) <<  8) |"
-		       "	(((unsigned int)(x) & (unsigned int)0x00ff0000UL) >>  8) |"
-		       "	(((unsigned int)(x) & (unsigned int)0xff000000UL) >> 24)))\n");
-	add_pre_buffer("#define __sparse_constant_swab64(x) ((unsigned long long)("
-		       "	(((unsigned long long)(x) & (unsigned long long)0x00000000000000ffULL) << 56) |"
-		       "	(((unsigned long long)(x) & (unsigned long long)0x000000000000ff00ULL) << 40) |"
-		       "	(((unsigned long long)(x) & (unsigned long long)0x0000000000ff0000ULL) << 24) |"
-		       "	(((unsigned long long)(x) & (unsigned long long)0x00000000ff000000ULL) <<  8) |"
-		       "	(((unsigned long long)(x) & (unsigned long long)0x000000ff00000000ULL) >>  8) |"
-		       "	(((unsigned long long)(x) & (unsigned long long)0x0000ff0000000000ULL) >> 24) |"
-		       "	(((unsigned long long)(x) & (unsigned long long)0x00ff000000000000ULL) >> 40) |"
-		       "	(((unsigned long long)(x) & (unsigned long long)0xff00000000000000ULL) >> 56)))\n");
-	add_pre_buffer("#define __builtin_bswap16(x)"
-		       "	(__builtin_constant_p((unsigned short)(x)) ?"
-		       "	__sparse_constant_swab16(x) :"
-		       "	____builtin_bswap16(x))\n");
-	add_pre_buffer("#define __builtin_bswap32(x)"
-		       "	(__builtin_constant_p((unsigned int)(x)) ?"
-		       "	__sparse_constant_swab32(x) :"
-		       "	____builtin_bswap32(x))\n");
-	add_pre_buffer("#define __builtin_bswap64(x)"
-		       "	(__builtin_constant_p((unsigned long long)(x)) ?"
-		       "	__sparse_constant_swab64(x) :"
-		       "	____builtin_bswap64(x))\n");
-
 	/* And atomic memory access functions.. */
 	add_pre_buffer("extern int __sync_fetch_and_add(void *, ...);\n");
 	add_pre_buffer("extern int __sync_fetch_and_sub(void *, ...);\n");
diff --git a/lib.h b/lib.h
index b778bdc..306ee45 100644
--- a/lib.h
+++ b/lib.h
@@ -200,6 +200,11 @@ static inline struct instruction *first_instruction(struct instruction_list *hea
 	return first_ptr_list((struct ptr_list *)head);
 }
 
+static inline struct expression *first_expression(struct expression_list *head)
+{
+	return first_ptr_list((struct ptr_list *)head);
+}
+
 static inline pseudo_t first_pseudo(struct pseudo_list *head)
 {
 	return first_ptr_list((struct ptr_list *)head);
diff --git a/symbol.c b/symbol.c
index 92a7a62..e57f207 100644
--- a/symbol.c
+++ b/symbol.c
@@ -773,6 +773,11 @@ static struct symbol_op choose_op = {
 	.args = arguments_choose,
 };
 
+static struct symbol_op bswap_op = {
+	.evaluate = evaluate_to_integer,
+	.expand = expand_bswap
+};
+
 /*
  * Builtin functions
  */
@@ -788,6 +793,9 @@ static struct sym_init {
 	{ "__builtin_warning", &builtin_fn_type, MOD_TOPLEVEL, &warning_op },
 	{ "__builtin_expect", &builtin_fn_type, MOD_TOPLEVEL, &expect_op },
 	{ "__builtin_choose_expr", &builtin_fn_type, MOD_TOPLEVEL, &choose_op },
+	{ "__builtin_bswap16", &builtin_fn_type, MOD_TOPLEVEL, &bswap_op },
+	{ "__builtin_bswap32", &builtin_fn_type, MOD_TOPLEVEL, &bswap_op },
+	{ "__builtin_bswap64", &builtin_fn_type, MOD_TOPLEVEL, &bswap_op },
 	{ NULL,		NULL,		0 }
 };
 
diff --git a/symbol.h b/symbol.h
index 9b3f160..48bbfce 100644
--- a/symbol.h
+++ b/symbol.h
@@ -130,6 +130,7 @@ struct symbol_op {
 
 extern int expand_safe_p(struct expression *expr, int cost);
 extern int expand_constant_p(struct expression *expr, int cost);
+extern int expand_bswap(struct expression *expr, int cost);
 
 #define SYM_ATTR_WEAK		0
 #define SYM_ATTR_NORMAL		1

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] implement constant-folding in __builtin_bswap*()
  2016-11-24  0:56                 ` Christopher Li
  2016-11-24  0:58                   ` Christopher Li
@ 2016-11-24  1:31                   ` Luc Van Oostenryck
  2016-11-24  3:23                     ` Christopher Li
  1 sibling, 1 reply; 24+ messages in thread
From: Luc Van Oostenryck @ 2016-11-24  1:31 UTC (permalink / raw)
  To: Christopher Li; +Cc: Johannes Berg, Linux-Sparse, Nicolai Stange

On Thu, Nov 24, 2016 at 08:56:45AM +0800, Christopher Li wrote:
> On Thu, Nov 24, 2016 at 4:48 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> > There is a problem though, with non-constant args.
> > Now the predeclarations of the 3 __builtin_bswap16/32/64
> > are gone, no more prototype and thus no more typing information.
> >
> > For example, if you call __builtin_bswap64() with an int argument,
> > there is no way for the next steps to know the arg must first
> > be 64 bit extended. Same for the result's type.
> 
> That is a very good point. Need more work on that.
> 
> The currently problem if I include the function prototype, the
> function prototype symbol will over shadow the symbol contain
> swap_op. We need to make sure the function prototype symbol
> is the one loading with correct swap_op.

Yes indeed.

Maybe another approach is possible:
- note the fact that "stuff" (I intentionally don't want to use
  "function") like __builtin_bswap*() behave like an operator
  or a function in the mathematical sense: they only depend on
  their args, have no side-effect, ... in others word they are "pure"
  like in MOD_PURE.
- because they're pure they necessarily return constant value if given
  constant args. This may be used early, the value itself is only needed
  in later phases.

It may also not be a bad idea to have a specific instructions for these
(like a few others, I'm thinking to rotate), after all it's not without
reasons that common CPU archs have these as real instructions
(but this won't directly help our present problem).
 
> Ideally, all the "__builtin_xxx" related stuff should be move to a
> separate builtin.c. Right now the function prototype is in lib.c
> The implementation is sprinkle around symbol.c, evaluation.c
> and expand.c.

It would be good, yes.
 
Luc

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] implement constant-folding in __builtin_bswap*()
  2016-11-24  0:58                   ` Christopher Li
@ 2016-11-24  1:36                     ` Luc Van Oostenryck
  2016-11-24  3:17                       ` Christopher Li
  0 siblings, 1 reply; 24+ messages in thread
From: Luc Van Oostenryck @ 2016-11-24  1:36 UTC (permalink / raw)
  To: Christopher Li; +Cc: Johannes Berg, Linux-Sparse, Nicolai Stange

On Thu, Nov 24, 2016 at 08:58:35AM +0800, Christopher Li wrote:
> +/* The arguments are constant if the cost of all of them is zero */
> +int expand_bswap(struct expression *expr, int cost)
> +{
> +	struct symbol *sym;
> +	struct expression_list *args = expr->args;
> +	long long input;
> +	int count;
> +
> +	if (cost)
> +		return cost;
> +
> +	sym = expr->fn->ctype;
> +	count = expression_list_size(args);
> +	if (count != 1) {
> +		sparse_error(expr->pos, "Invaild number of arguments for function %s (expected 1 got %d)",

Small typo here: "Invaild"

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] implement constant-folding in __builtin_bswap*()
  2016-11-24  1:36                     ` Luc Van Oostenryck
@ 2016-11-24  3:17                       ` Christopher Li
  0 siblings, 0 replies; 24+ messages in thread
From: Christopher Li @ 2016-11-24  3:17 UTC (permalink / raw)
  To: Luc Van Oostenryck; +Cc: Johannes Berg, Linux-Sparse, Nicolai Stange

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

On Thu, Nov 24, 2016 at 9:36 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> Small typo here: "Invaild"

Good catch. The corrected patch follows:

Chris

[-- Attachment #2: implement-builtin-bswap-2.patch --]
[-- Type: text/x-patch, Size: 5948 bytes --]







--- sparse.chrisl.orig/symbol.h
+++ sparse.chrisl/symbol.h
@@ -130,6 +130,7 @@ struct symbol_op {
 
 extern int expand_safe_p(struct expression *expr, int cost);
 extern int expand_constant_p(struct expression *expr, int cost);
+extern int expand_bswap(struct expression *expr, int cost);
 
 #define SYM_ATTR_WEAK		0
 #define SYM_ATTR_NORMAL		1
--- sparse.chrisl.orig/lib.c
+++ sparse.chrisl/lib.c
@@ -819,40 +819,6 @@ void declare_builtin_functions(void)
 	add_pre_buffer("extern int __builtin_popcountl(unsigned long);\n");
 	add_pre_buffer("extern int __builtin_popcountll(unsigned long long);\n");
 
-	/* And byte swaps.. */
-	add_pre_buffer("extern unsigned short ____builtin_bswap16(unsigned short);\n");
-	add_pre_buffer("extern unsigned int ____builtin_bswap32(unsigned int);\n");
-	add_pre_buffer("extern unsigned long long ____builtin_bswap64(unsigned long long);\n");
-	add_pre_buffer("#define __sparse_constant_swab16(x) ((unsigned short)("
-		       "	(((unsigned short)(x) & (unsigned short)0x00ffU) << 8) |"
-		       "	(((unsigned short)(x) & (unsigned short)0xff00U) >> 8)))\n");
-	add_pre_buffer("#define __sparse_constant_swab32(x) ((unsigned int)("
-		       "	(((unsigned int)(x) & (unsigned int)0x000000ffUL) << 24) |"
-		       "	(((unsigned int)(x) & (unsigned int)0x0000ff00UL) <<  8) |"
-		       "	(((unsigned int)(x) & (unsigned int)0x00ff0000UL) >>  8) |"
-		       "	(((unsigned int)(x) & (unsigned int)0xff000000UL) >> 24)))\n");
-	add_pre_buffer("#define __sparse_constant_swab64(x) ((unsigned long long)("
-		       "	(((unsigned long long)(x) & (unsigned long long)0x00000000000000ffULL) << 56) |"
-		       "	(((unsigned long long)(x) & (unsigned long long)0x000000000000ff00ULL) << 40) |"
-		       "	(((unsigned long long)(x) & (unsigned long long)0x0000000000ff0000ULL) << 24) |"
-		       "	(((unsigned long long)(x) & (unsigned long long)0x00000000ff000000ULL) <<  8) |"
-		       "	(((unsigned long long)(x) & (unsigned long long)0x000000ff00000000ULL) >>  8) |"
-		       "	(((unsigned long long)(x) & (unsigned long long)0x0000ff0000000000ULL) >> 24) |"
-		       "	(((unsigned long long)(x) & (unsigned long long)0x00ff000000000000ULL) >> 40) |"
-		       "	(((unsigned long long)(x) & (unsigned long long)0xff00000000000000ULL) >> 56)))\n");
-	add_pre_buffer("#define __builtin_bswap16(x)"
-		       "	(__builtin_constant_p((unsigned short)(x)) ?"
-		       "	__sparse_constant_swab16(x) :"
-		       "	____builtin_bswap16(x))\n");
-	add_pre_buffer("#define __builtin_bswap32(x)"
-		       "	(__builtin_constant_p((unsigned int)(x)) ?"
-		       "	__sparse_constant_swab32(x) :"
-		       "	____builtin_bswap32(x))\n");
-	add_pre_buffer("#define __builtin_bswap64(x)"
-		       "	(__builtin_constant_p((unsigned long long)(x)) ?"
-		       "	__sparse_constant_swab64(x) :"
-		       "	____builtin_bswap64(x))\n");
-
 	/* And atomic memory access functions.. */
 	add_pre_buffer("extern int __sync_fetch_and_add(void *, ...);\n");
 	add_pre_buffer("extern int __sync_fetch_and_sub(void *, ...);\n");
--- sparse.chrisl.orig/ident-list.h
+++ sparse.chrisl/ident-list.h
@@ -35,6 +35,9 @@ IDENT_RESERVED(__sizeof_ptr__);
 IDENT_RESERVED(__builtin_types_compatible_p);
 IDENT_RESERVED(__builtin_offsetof);
 IDENT_RESERVED(__label__);
+IDENT(__builtin_bswap16);
+IDENT(__builtin_bswap32);
+IDENT(__builtin_bswap64);
 
 /* Attribute names */
 IDENT(packed); IDENT(__packed__);
--- sparse.chrisl.orig/symbol.c
+++ sparse.chrisl/symbol.c
@@ -773,6 +773,11 @@ static struct symbol_op choose_op = {
 	.args = arguments_choose,
 };
 
+static struct symbol_op bswap_op = {
+	.evaluate = evaluate_to_integer,
+	.expand = expand_bswap
+};
+
 /*
  * Builtin functions
  */
@@ -788,6 +793,9 @@ static struct sym_init {
 	{ "__builtin_warning", &builtin_fn_type, MOD_TOPLEVEL, &warning_op },
 	{ "__builtin_expect", &builtin_fn_type, MOD_TOPLEVEL, &expect_op },
 	{ "__builtin_choose_expr", &builtin_fn_type, MOD_TOPLEVEL, &choose_op },
+	{ "__builtin_bswap16", &builtin_fn_type, MOD_TOPLEVEL, &bswap_op },
+	{ "__builtin_bswap32", &builtin_fn_type, MOD_TOPLEVEL, &bswap_op },
+	{ "__builtin_bswap64", &builtin_fn_type, MOD_TOPLEVEL, &bswap_op },
 	{ NULL,		NULL,		0 }
 };
 
--- sparse.chrisl.orig/lib.h
+++ sparse.chrisl/lib.h
@@ -200,6 +200,11 @@ static inline struct instruction *first_
 	return first_ptr_list((struct ptr_list *)head);
 }
 
+static inline struct expression *first_expression(struct expression_list *head)
+{
+	return first_ptr_list((struct ptr_list *)head);
+}
+
 static inline pseudo_t first_pseudo(struct pseudo_list *head)
 {
 	return first_ptr_list((struct ptr_list *)head);
--- sparse.chrisl.orig/expand.c
+++ sparse.chrisl/expand.c
@@ -802,6 +802,46 @@ int expand_safe_p(struct expression *exp
 	return 0;
 }
 
+/* The arguments are constant if the cost of all of them is zero */
+int expand_bswap(struct expression *expr, int cost)
+{
+	struct symbol *sym;
+	struct expression_list *args = expr->args;
+	long long input;
+	int count;
+
+	if (cost)
+		return cost;
+
+	sym = expr->fn->ctype;
+	count = expression_list_size(args);
+	if (count != 1) {
+		sparse_error(expr->pos, "Invalid number of arguments for function %s (expected 1 got %d)",
+				show_ident(sym->ident), count);
+		return SIDE_EFFECTS;
+	}
+
+	input = const_expression_value(first_expression(args));
+
+	if (sym->ident == &__builtin_bswap16_ident) {
+		expr->value = __builtin_bswap16(input);
+		expr->ctype = &ushort_ctype;
+	} else if (sym->ident == &__builtin_bswap32_ident) {
+		expr->value = __builtin_bswap32(input);
+		expr->ctype = &uint_ctype;
+	} else if (sym->ident == &__builtin_bswap64_ident) {
+		expr->value = __builtin_bswap64(input);
+		expr->ctype = &ullong_ctype;
+	} else {
+		die("Unexpected __builtin_bswap symbol %s\n", show_ident(sym->ident));
+	}
+
+	expr->type = EXPR_VALUE;
+	expr->taint = 0;
+	return 0;
+}
+
+
 /*
  * expand a call expression with a symbol. This
  * should expand builtins.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] implement constant-folding in __builtin_bswap*()
  2016-11-24  1:31                   ` Luc Van Oostenryck
@ 2016-11-24  3:23                     ` Christopher Li
  0 siblings, 0 replies; 24+ messages in thread
From: Christopher Li @ 2016-11-24  3:23 UTC (permalink / raw)
  To: Luc Van Oostenryck; +Cc: Johannes Berg, Linux-Sparse, Nicolai Stange

On Thu, Nov 24, 2016 at 9:31 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> Maybe another approach is possible:
> - note the fact that "stuff" (I intentionally don't want to use
>   "function") like __builtin_bswap*() behave like an operator
>   or a function in the mathematical sense: they only depend on
>   their args, have no side-effect, ... in others word they are "pure"
>   like in MOD_PURE.
> - because they're pure they necessarily return constant value if given
>   constant args. This may be used early, the value itself is only needed
>   in later phases.

I think that is exactly why __builtins_xxx exist. There is also pure
function and inline function etc. I guess I will still call it a function
rather than "stuff".

> It may also not be a bad idea to have a specific instructions for these
> (like a few others, I'm thinking to rotate), after all it's not without
> reasons that common CPU archs have these as real instructions
> (but this won't directly help our present problem).

If the function is expanded at compile time. It does not matter what
instruction it was used. It only matter when it need to emit as real
instruction in the back end. The black end still have the flexibility
to inline asm it.

Agree, it does not help our current problem though.

Chris

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] add test case for builtin bswap with constant args
  2016-11-23 20:38               ` [PATCH] add test case for builtin bswap with constant args Luc Van Oostenryck
  2016-11-24  0:17                 ` Christopher Li
@ 2016-11-24  5:30                 ` Christopher Li
  2016-11-24 13:00                   ` Luc Van Oostenryck
  1 sibling, 1 reply; 24+ messages in thread
From: Christopher Li @ 2016-11-24  5:30 UTC (permalink / raw)
  To: Luc Van Oostenryck; +Cc: Linux-Sparse

On Thu, Nov 24, 2016 at 4:38 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> ---
>  validation/builtin-constant-eval.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 validation/builtin-constant-eval.c

Can you reply with a sign off line?

Thanks

Chris

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] add test case for builtin bswap with constant args
  2016-11-24  5:30                 ` Christopher Li
@ 2016-11-24 13:00                   ` Luc Van Oostenryck
  0 siblings, 0 replies; 24+ messages in thread
From: Luc Van Oostenryck @ 2016-11-24 13:00 UTC (permalink / raw)
  To: Christopher Li; +Cc: Linux-Sparse

On Thu, Nov 24, 2016 at 01:30:14PM +0800, Christopher Li wrote:
> On Thu, Nov 24, 2016 at 4:38 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> > ---
> >  validation/builtin-constant-eval.c | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >  create mode 100644 validation/builtin-constant-eval.c
> 
> Can you reply with a sign off line?
> 
> Thanks
> 
> Chris
> --

Oh yes, I forgot this. Sorry.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2016-11-24 13:00 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-17  9:55 [PATCH v2] implement constant-folding in __builtin_bswap*() Johannes Berg
2016-11-21  2:38 ` Christopher Li
2016-11-21  9:45   ` Johannes Berg
2016-11-22 11:13     ` Christopher Li
2016-11-22 11:39       ` Christopher Li
2016-11-22 13:15       ` Luc Van Oostenryck
2016-11-22 16:32         ` Christopher Li
2016-11-22 17:12           ` Luc Van Oostenryck
2016-11-23  1:23             ` Christopher Li
2016-11-22 20:16           ` Luc Van Oostenryck
2016-11-23  1:25             ` Christopher Li
2016-11-23 20:38               ` [PATCH] add test case for builtin bswap with constant args Luc Van Oostenryck
2016-11-24  0:17                 ` Christopher Li
2016-11-24  5:30                 ` Christopher Li
2016-11-24 13:00                   ` Luc Van Oostenryck
2016-11-23 20:48               ` [PATCH v2] implement constant-folding in __builtin_bswap*() Luc Van Oostenryck
2016-11-24  0:56                 ` Christopher Li
2016-11-24  0:58                   ` Christopher Li
2016-11-24  1:36                     ` Luc Van Oostenryck
2016-11-24  3:17                       ` Christopher Li
2016-11-24  1:31                   ` Luc Van Oostenryck
2016-11-24  3:23                     ` Christopher Li
2016-11-23  6:28           ` Johannes Berg
2016-11-23 14:30             ` Christopher Li

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.