diff mbox

[v2] implement constant-folding in __builtin_bswap*()

Message ID CANeU7QmSd5Zd_H2jDQyTjUMg7C4_6wQOyCR73R=FTdJ=f4+ygg@mail.gmail.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Christopher Li Nov. 22, 2016, 4:32 p.m. UTC
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

Comments

Luc Van Oostenryck Nov. 22, 2016, 5:12 p.m. UTC | #1
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
--
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
Luc Van Oostenryck Nov. 22, 2016, 8:16 p.m. UTC | #2
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>
--
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
Christopher Li Nov. 23, 2016, 1:23 a.m. UTC | #3
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
--
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
Christopher Li Nov. 23, 2016, 1:25 a.m. UTC | #4
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
--
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
Johannes Berg Nov. 23, 2016, 6:28 a.m. UTC | #5
> 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
--
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
Christopher Li Nov. 23, 2016, 2:30 p.m. UTC | #6
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
--
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
Luc Van Oostenryck Nov. 23, 2016, 8:48 p.m. UTC | #7
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
--
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
Christopher Li Nov. 24, 2016, 12:56 a.m. UTC | #8
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
--
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
Luc Van Oostenryck Nov. 24, 2016, 1:31 a.m. UTC | #9
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
--
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
Christopher Li Nov. 24, 2016, 3:23 a.m. UTC | #10
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
--
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
diff mbox

Patch

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