All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] normalize pseudos numbering
@ 2017-06-03  7:34 Luc Van Oostenryck
  2017-06-03  7:34 ` [PATCH 1/3] rename 'struct warning' to 'struct flag' Luc Van Oostenryck
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Luc Van Oostenryck @ 2017-06-03  7:34 UTC (permalink / raw)
  To: linux-sparse; +Cc: Chris Li, Luc Van Oostenryck

The goal of this series is to add a phase of renumbering
of all pseudos so that the pseudos' name are much less
dependent on linearization and optimization details
and thus make the output of test-linearize much more suited
for the test cases.

Luc Van Oostenryck (3):
  rename 'struct warning' to 'struct flag'
  let handle_simple_switch() handle an array of flags
  allow to normalize the pseudos

 Makefile         |   1 +
 lib.c            |  42 ++++++++++++--------
 lib.h            |   1 +
 linearize.h      |   1 +
 norm-pseudo.c    | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 test-linearize.c |   2 +
 test-unssa.c     |   2 +
 7 files changed, 149 insertions(+), 17 deletions(-)
 create mode 100644 norm-pseudo.c

-- 
2.13.0


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

* [PATCH 1/3] rename 'struct warning' to 'struct flag'
  2017-06-03  7:34 [PATCH 0/3] normalize pseudos numbering Luc Van Oostenryck
@ 2017-06-03  7:34 ` Luc Van Oostenryck
  2017-06-03  7:34 ` [PATCH 2/3] let handle_simple_switch() handle an array of flags Luc Van Oostenryck
  2017-06-03  7:34 ` [PATCH 3/3] allow to normalize the pseudos Luc Van Oostenryck
  2 siblings, 0 replies; 9+ messages in thread
From: Luc Van Oostenryck @ 2017-06-03  7:34 UTC (permalink / raw)
  To: linux-sparse; +Cc: Chris Li, Luc Van Oostenryck

'struct warning' can be reused for flags other than warnings.

To avoid future confusion, rename it to something more general:
'struct flag' (which in its context, handling of compiler flags,
is clear enough).

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 lib.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib.c b/lib.c
index e1e6a1cbf..569c00e97 100644
--- a/lib.c
+++ b/lib.c
@@ -488,7 +488,7 @@ static char **handle_switch_o(char *arg, char **next)
 	return next;
 }
 
-static const struct warning {
+static const struct flag {
 	const char *name;
 	int *flag;
 } warnings[] = {
@@ -532,7 +532,7 @@ enum {
 };
 
 
-static char **handle_onoff_switch(char *arg, char **next, const struct warning warnings[], int n)
+static char **handle_onoff_switch(char *arg, char **next, const struct flag warnings[], int n)
 {
 	int flag = WARNING_ON;
 	char *p = arg + 1;
@@ -574,7 +574,7 @@ static char **handle_switch_W(char *arg, char **next)
 	return next;
 }
 
-static struct warning debugs[] = {
+static struct flag debugs[] = {
 	{ "entry", &dbg_entry},
 	{ "dead", &dbg_dead},
 };
@@ -593,7 +593,7 @@ static char **handle_switch_v(char *arg, char **next)
 	return next;
 }
 
-static struct warning dumps[] = {
+static struct flag dumps[] = {
 	{ "D", &dump_macro_defs},
 };
 
@@ -607,7 +607,7 @@ static char **handle_switch_d(char *arg, char **next)
 }
 
 
-static void handle_onoff_switch_finalize(const struct warning warnings[], int n)
+static void handle_onoff_switch_finalize(const struct flag warnings[], int n)
 {
 	unsigned i;
 
-- 
2.13.0


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

* [PATCH 2/3] let handle_simple_switch() handle an array of flags
  2017-06-03  7:34 [PATCH 0/3] normalize pseudos numbering Luc Van Oostenryck
  2017-06-03  7:34 ` [PATCH 1/3] rename 'struct warning' to 'struct flag' Luc Van Oostenryck
@ 2017-06-03  7:34 ` Luc Van Oostenryck
  2017-06-03  7:34 ` [PATCH 3/3] allow to normalize the pseudos Luc Van Oostenryck
  2 siblings, 0 replies; 9+ messages in thread
From: Luc Van Oostenryck @ 2017-06-03  7:34 UTC (permalink / raw)
  To: linux-sparse; +Cc: Chris Li, Luc Van Oostenryck

This was used to handle a single flag but we need something
more compact when we need to handle several flags.

So, adapt this helper so that it now takes an array of flags
instead of a single flag.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 lib.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/lib.c b/lib.c
index 569c00e97..a38a6f622 100644
--- a/lib.c
+++ b/lib.c
@@ -458,7 +458,12 @@ static void handle_arch_finalize(void)
 }
 
 
-static int handle_simple_switch(const char *arg, const char *name, int *flag)
+struct flag {
+	const char *name;
+	int *flag;
+};
+
+static int handle_simple_switch(const char *arg, const struct flag *flags, unsigned int n)
 {
 	int val = 1;
 
@@ -468,9 +473,11 @@ static int handle_simple_switch(const char *arg, const char *name, int *flag)
 		val = 0;
 	}
 
-	if (strcmp(arg, name) == 0) {
-		*flag = val;
-		return 1;
+	for (; n--; flags++) {
+		if (strcmp(arg, flags->name) == 0) {
+			*flags->flag = val;
+			return 1;
+		}
 	}
 
 	// not handled
@@ -488,10 +495,7 @@ static char **handle_switch_o(char *arg, char **next)
 	return next;
 }
 
-static const struct flag {
-	const char *name;
-	int *flag;
-} warnings[] = {
+static const struct flag warnings[] = {
 	{ "address", &Waddress },
 	{ "address-space", &Waddress_space },
 	{ "bitwise", &Wbitwise },
@@ -703,19 +707,21 @@ err:
 	die("error: unknown flag \"-fdump-%s\"", arg);
 }
 
+static struct flag fflags[] = {
+	{ "mem-report", &fmem_report },
+};
+
 static char **handle_switch_f(char *arg, char **next)
 {
 	arg++;
 
+	if (handle_simple_switch(arg, fflags, ARRAY_SIZE(fflags)))
+		return next;
+
 	if (!strncmp(arg, "tabstop=", 8))
 		return handle_switch_ftabstop(arg+8, next);
 	if (!strncmp(arg, "dump-", 5))
 		return handle_switch_fdump(arg+5, next);
-
-	/* handle switches w/ arguments above, boolean and only boolean below */
-	if (handle_simple_switch(arg, "mem-report", &fmem_report))
-		return next;

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

* [PATCH 3/3] allow to normalize the pseudos
  2017-06-03  7:34 [PATCH 0/3] normalize pseudos numbering Luc Van Oostenryck
  2017-06-03  7:34 ` [PATCH 1/3] rename 'struct warning' to 'struct flag' Luc Van Oostenryck
  2017-06-03  7:34 ` [PATCH 2/3] let handle_simple_switch() handle an array of flags Luc Van Oostenryck
@ 2017-06-03  7:34 ` Luc Van Oostenryck
  2017-06-05 18:45   ` Christopher Li
  2 siblings, 1 reply; 9+ messages in thread
From: Luc Van Oostenryck @ 2017-06-03  7:34 UTC (permalink / raw)
  To: linux-sparse; +Cc: Chris Li, Luc Van Oostenryck

Using the output of test-linearize for testing is not
ideal because some details of this output are very dependent
to small changes in linearization or optimization while the
test in itself may very well not concerned about those changes.

One of the things that are particulary sensitive to these changes
is the number of the pseudos (%r123, ...).

Make these tests much less sensitive to these changes by adding
a normalization phase called just before emitting the output.
This normalization will renumber all the pseudos sequentially,
restarting at %r1 at each functions.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 Makefile         |   1 +
 lib.c            |   2 +
 lib.h            |   1 +
 linearize.h      |   1 +
 norm-pseudo.c    | 117 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 test-linearize.c |   2 +
 test-unssa.c     |   2 +
 7 files changed, 126 insertions(+)
 create mode 100644 norm-pseudo.c

diff --git a/Makefile b/Makefile
index b32fc96ea..dee432e0e 100644
--- a/Makefile
+++ b/Makefile
@@ -106,6 +106,7 @@ LIB_OBJS= target.o parse.o tokenize.o pre-process.o symbol.o lib.o scope.o \
 	  expression.o show-parse.o evaluate.o expand.o inline.o linearize.o \
 	  char.o sort.o allocate.o compat-$(OS).o ptrlist.o \
 	  builtin.o \
+	  norm-pseudo.o \
 	  stats.o \
 	  flow.o cse.o simplify.o memops.o liveness.o storage.o unssa.o dissect.o
 
diff --git a/lib.c b/lib.c
index a38a6f622..4f5d06849 100644
--- a/lib.c
+++ b/lib.c
@@ -255,6 +255,7 @@ int dbg_dead = 0;
 
 int fmem_report = 0;
 int fdump_linearize;
+int fnormalize_pseudos = 0;
 
 int preprocess_only;
 
@@ -709,6 +710,7 @@ err:
 
 static struct flag fflags[] = {
 	{ "mem-report", &fmem_report },
+	{ "normalize-pseudos", &fnormalize_pseudos },
 };
 
 static char **handle_switch_f(char *arg, char **next)
diff --git a/lib.h b/lib.h
index 2c8529f93..b6cb34308 100644
--- a/lib.h
+++ b/lib.h
@@ -142,6 +142,7 @@ extern int dbg_dead;
 
 extern int fmem_report;
 extern int fdump_linearize;
+extern int fnormalize_pseudos;
 
 extern int arch_m64;
 
diff --git a/linearize.h b/linearize.h
index bac82d7ff..cbd66187f 100644
--- a/linearize.h
+++ b/linearize.h
@@ -341,6 +341,7 @@ void show_entry(struct entrypoint *ep);
 const char *show_pseudo(pseudo_t pseudo);
 void show_bb(struct basic_block *bb);
 const char *show_instruction(struct instruction *insn);
+void normalize_pseudos(struct entrypoint *ep);
 
 #endif /* LINEARIZE_H */
 
diff --git a/norm-pseudo.c b/norm-pseudo.c
new file mode 100644
index 000000000..920841c49
--- /dev/null
+++ b/norm-pseudo.c
@@ -0,0 +1,117 @@
+#include "linearize.h"
+
+static int pseudo_nr;
+
+static void norm_pseudo(pseudo_t pseudo, int init)
+{
+	if (!pseudo)
+		return;
+	if (pseudo->type != PSEUDO_REG)
+		return;
+	if (init) {
+		if (!pseudo->nr)
+			return;
+		pseudo->nr = 0;
+	} else {
+		if (pseudo->nr)
+			return;
+		pseudo->nr = ++pseudo_nr;
+	}
+}
+
+static void norm_asm_constraints(struct asm_constraint_list *list, int init)
+{
+	struct asm_constraint *entry;
+
+	FOR_EACH_PTR(list, entry) {
+		norm_pseudo(entry->pseudo, init);
+	} END_FOR_EACH_PTR(entry);
+}
+
+static void norm_insn(struct instruction *insn, int init)
+{
+	switch (insn->opcode) {
+	case OP_SEL:
+	case OP_RANGE:
+		norm_pseudo(insn->src3, init);
+		/* fall through */
+
+	case OP_BINARY ... OP_BINARY_END:
+	case OP_BINCMP ... OP_BINCMP_END:
+		norm_pseudo(insn->src2, init);
+		/* fall through */
+
+	case OP_LOAD: case OP_LNOP:
+	case OP_STORE: case OP_SNOP:
+	case OP_CAST:
+	case OP_SCAST:
+	case OP_FPCAST:
+	case OP_PTRCAST:
+	case OP_NOT: case OP_NEG:
+	case OP_COPY:
+	case OP_PHISOURCE:
+	case OP_RET:
+	case OP_SLICE:
+		norm_pseudo(insn->src, init);
+		/* fall through */
+
+	case OP_PHI:
+	case OP_SYMADDR:
+	case OP_SETVAL:
+	case OP_DEATHNOTE:
+	case OP_CBR:
+	case OP_SWITCH:
+	case OP_COMPUTEDGOTO:
+		norm_pseudo(insn->target, init);
+		break;
+
+	case OP_INLINED_CALL:
+	case OP_CALL: {
+		struct pseudo *arg;
+		FOR_EACH_PTR(insn->arguments, arg) {
+			norm_pseudo(arg, init);
+		} END_FOR_EACH_PTR(arg);
+		norm_pseudo(insn->target, init);
+		break;
+	}
+
+	case OP_ASM:
+		if (!insn->asm_rules)
+			break;
+		norm_asm_constraints(insn->asm_rules->outputs, init);
+		norm_asm_constraints(insn->asm_rules->inputs, init);
+		norm_asm_constraints(insn->asm_rules->clobbers, init);
+		break;
+
+	case OP_NOP:
+		break;
+
+	default:
+		break;
+	}
+}
+
+static void norm_ep(struct entrypoint *ep, int init)
+{
+	struct basic_block *bb;
+	struct instruction *insn;
+
+	FOR_EACH_PTR(ep->bbs, bb) {
+		if (!bb->ep)
+			continue;
+		FOR_EACH_PTR(bb->insns, insn) {
+			if (!insn->bb)
+				continue;
+			norm_insn(insn, init);
+		} END_FOR_EACH_PTR(bb);
+	} END_FOR_EACH_PTR(bb);
+}
+
+void normalize_pseudos(struct entrypoint *ep)
+{
+	if (!ep)
+		return;
+	norm_ep(ep, 1);
+	pseudo_nr = 0;
+	norm_ep(ep, 0);
+}
diff --git a/test-linearize.c b/test-linearize.c
index fe0673bef..2bcfb9833 100644
--- a/test-linearize.c
+++ b/test-linearize.c
@@ -47,6 +47,8 @@ static void clean_up_symbols(struct symbol_list *list)
 
 		expand_symbol(sym);
 		ep = linearize_symbol(sym);
+		if (fnormalize_pseudos)
+			normalize_pseudos(ep);
 		if (ep)
 			show_entry(ep);
 	} END_FOR_EACH_PTR(sym);
diff --git a/test-unssa.c b/test-unssa.c
index 240d99601..900163aa4 100644
--- a/test-unssa.c
+++ b/test-unssa.c
@@ -37,6 +37,8 @@ static void output_fn(struct entrypoint *ep)
 		printf("\n\n.globl %s\n%s:\n", name, name);
 
 	unssa(ep);
+	if (fnormalize_pseudos)
+		normalize_pseudos(ep);
 
 	FOR_EACH_PTR(ep->bbs, bb) {
 		if (bb->generation == generation)
-- 
2.13.0


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

* Re: [PATCH 3/3] allow to normalize the pseudos
  2017-06-03  7:34 ` [PATCH 3/3] allow to normalize the pseudos Luc Van Oostenryck
@ 2017-06-05 18:45   ` Christopher Li
  2017-06-05 19:54     ` Luc Van Oostenryck
  0 siblings, 1 reply; 9+ messages in thread
From: Christopher Li @ 2017-06-05 18:45 UTC (permalink / raw)
  To: Luc Van Oostenryck; +Cc: Linux-Sparse

On Sat, Jun 3, 2017 at 12:34 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> Using the output of test-linearize for testing is not
> ideal because some details of this output are very dependent
> to small changes in linearization or optimization while the
> test in itself may very well not concerned about those changes.
>
> One of the things that are particulary sensitive to these changes
> is the number of the pseudos (%r123, ...).
>
> Make these tests much less sensitive to these changes by adding
> a normalization phase called just before emitting the output.
> This normalization will renumber all the pseudos sequentially,
> restarting at %r1 at each functions.

I have a totally untested idea might be able to simplify this
normalize process. It seems most of the pseudo output is
come from show_pseudo(). We can use the generation number
idea. All we need to do is just keep track of the base of the pseudo
number. If you need new series of number just set the base to a number bigger
than all of the pseudo number. That might means make alloc_pseudo::nr
visible to other function we know what is the max pseudo number.

Initialize the normalization is easy, just set
     pseudo_base = ++pseudo_max;

Then in show_pseudo() we can do the normalize as following:

if (pseudo->nr - pseudo_base < 0)
     pseudo->nr = ++pseudo_max;

For the output of the number, we have:
nr = pseudo->nr - pseudo_base;


One problem I can see if the pseudo->nr wrap around then it
will create confusion. Maybe not some thing we need to worry
about any time soon. Another thing is the pseudo->nr only
make sense in the function scope. We might able to use that
to handle overflowing if it really became a problem.

Might have some other problem as I haven't try to code it myself.

Chris

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

* Re: [PATCH 3/3] allow to normalize the pseudos
  2017-06-05 18:45   ` Christopher Li
@ 2017-06-05 19:54     ` Luc Van Oostenryck
  2017-06-05 20:50       ` Christopher Li
  0 siblings, 1 reply; 9+ messages in thread
From: Luc Van Oostenryck @ 2017-06-05 19:54 UTC (permalink / raw)
  To: Christopher Li; +Cc: Linux-Sparse

On Mon, Jun 5, 2017 at 8:45 PM, Christopher Li <sparse@chrisli.org> wrote:
> On Sat, Jun 3, 2017 at 12:34 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
>> Using the output of test-linearize for testing is not
>> ideal because some details of this output are very dependent
>> to small changes in linearization or optimization while the
>> test in itself may very well not concerned about those changes.
>>
>> One of the things that are particulary sensitive to these changes
>> is the number of the pseudos (%r123, ...).
>>
>> Make these tests much less sensitive to these changes by adding
>> a normalization phase called just before emitting the output.
>> This normalization will renumber all the pseudos sequentially,
>> restarting at %r1 at each functions.
>
> I have a totally untested idea might be able to simplify this
> normalize process. It seems most of the pseudo output is
> come from show_pseudo(). We can use the generation number
> idea. All we need to do is just keep track of the base of the pseudo
> number. If you need new series of number just set the base to a number bigger
> than all of the pseudo number. That might means make alloc_pseudo::nr
> visible to other function we know what is the max pseudo number.
>
> Initialize the normalization is easy, just set
>      pseudo_base = ++pseudo_max;
>
> Then in show_pseudo() we can do the normalize as following:
>
> if (pseudo->nr - pseudo_base < 0)
>      pseudo->nr = ++pseudo_max;
>
> For the output of the number, we have:
> nr = pseudo->nr - pseudo_base;

The problem that the patch here tries to solve is that *between* different
versions of sparse, because of some subtle (or not so subtle) changes
in the details of linearization or the optimization, a function, while
having essentially the same intermediate code have in fact different
name (here just the number) for its pseudos.
And these differences are annoying when comparing results, during
development and for the test suite. Also, given the 'problem', it's very
fine to keep the code simple and only make the changes in a separate
pass only used for dev & test.

I have already tried several things in the past 6 months and anything
a bit smart had some problems, it's why, finally, I opted for the brute force
here in this patch.

I think that what you propose here, will just allow to have consecutive
pseudo numbers but I don't see how it will give the guarantee that
"same code" will give "same pseudo numbers" and this will be easily
be 'diffed out' so that only real differences will show up in diff output.

> One problem I can see if the pseudo->nr wrap around then it
> will create confusion. Maybe not some thing we need to worry
> about any time soon.
Indeed, with even with 32bit ::nr, I'm not really worried :)

> Another thing is the pseudo->nr only
> make sense in the function scope. We might able to use that
> to handle overflowing if it really became a problem.
Well, it's true for now that we could reset the counter at each function
and this would already 'solve' most of the cases I had. But what if
we begin to add a second pass of automatic inlining?
And keeping unique names every pseudo has advantages.

-- Luc

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

* Re: [PATCH 3/3] allow to normalize the pseudos
  2017-06-05 19:54     ` Luc Van Oostenryck
@ 2017-06-05 20:50       ` Christopher Li
  2017-06-05 22:10         ` Luc Van Oostenryck
  0 siblings, 1 reply; 9+ messages in thread
From: Christopher Li @ 2017-06-05 20:50 UTC (permalink / raw)
  To: Luc Van Oostenryck; +Cc: Linux-Sparse

On Mon, Jun 5, 2017 at 12:54 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> The problem that the patch here tries to solve is that *between* different
> versions of sparse, because of some subtle (or not so subtle) changes
> in the details of linearization or the optimization, a function, while
> having essentially the same intermediate code have in fact different
> name (here just the number) for its pseudos.
> And these differences are annoying when comparing results, during
> development and for the test suite. Also, given the 'problem', it's very
> fine to keep the code simple and only make the changes in a separate
> pass only used for dev & test.
>
> I have already tried several things in the past 6 months and anything
> a bit smart had some problems, it's why, finally, I opted for the brute force
> here in this patch.
>
> I think that what you propose here, will just allow to have consecutive
> pseudo numbers but I don't see how it will give the guarantee that
> "same code" will give "same pseudo numbers" and this will be easily
> be 'diffed out' so that only real differences will show up in diff output.

You will still need to reset the pseudo_base per show_entry().
That way, each function is reset the displaying pseudo to start
from 1. Same code should have the same pseudo number at the
same instruction, no?

As far as I can tell, behavior should be very  similar to your patch.
Just don't need to do a separate pass to go through each instruction.

> Well, it's true for now that we could reset the counter at each function
> and this would already 'solve' most of the cases I had. But what if
> we begin to add a second pass of automatic inlining?

That is still fine, each pseudo has its own memory allocation.
The number is just for show_instruction text output. Make sure
reset the pseudo_base inside show_entry() will force the inlined function
allocate new pseudo for different pseudo, base on the memory allocation.
If after inline two different pseudo has the same "nr", as long as they
both smaller than pseudo_base, they will be assign to different number
 in the new round.

> And keeping unique names every pseudo has advantages.

I think each pseudo do has unique pointer and storage so that did not change.
The "nr" is only for showing the text information.

If we want to be smart about comparing the output, we should have
a smarter diff against instructions. Right now if one of the basic block
use one more pseudo will likely impact other basic block because the
pseudo has shifted for other basic block, even it is the instruction code
is equivalent for other basic block.

Chris

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

* Re: [PATCH 3/3] allow to normalize the pseudos
  2017-06-05 20:50       ` Christopher Li
@ 2017-06-05 22:10         ` Luc Van Oostenryck
  2017-06-06  2:42           ` Christopher Li
  0 siblings, 1 reply; 9+ messages in thread
From: Luc Van Oostenryck @ 2017-06-05 22:10 UTC (permalink / raw)
  To: Christopher Li; +Cc: Linux-Sparse

On Mon, Jun 5, 2017 at 10:50 PM, Christopher Li <sparse@chrisli.org> wrote:
> On Mon, Jun 5, 2017 at 12:54 PM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
>> The problem that the patch here tries to solve is that *between* different
>> versions of sparse, because of some subtle (or not so subtle) changes
>> in the details of linearization or the optimization, a function, while
>> having essentially the same intermediate code have in fact different
>> name (here just the number) for its pseudos.
>> And these differences are annoying when comparing results, during
>> development and for the test suite. Also, given the 'problem', it's very
>> fine to keep the code simple and only make the changes in a separate
>> pass only used for dev & test.
>>
>> I have already tried several things in the past 6 months and anything
>> a bit smart had some problems, it's why, finally, I opted for the brute force
>> here in this patch.
>>
>> I think that what you propose here, will just allow to have consecutive
>> pseudo numbers but I don't see how it will give the guarantee that
>> "same code" will give "same pseudo numbers" and this will be easily
>> be 'diffed out' so that only real differences will show up in diff output.
>
> You will still need to reset the pseudo_base per show_entry().
> That way, each function is reset the displaying pseudo to start
> from 1. Same code should have the same pseudo number at the
> same instruction, no?
>
> As far as I can tell, behavior should be very  similar to your patch.
> Just don't need to do a separate pass to go through each instruction.

It would fro the simple/normal cases but not in general because simplify
away an instruction and not creating this instruction at first don't have the
same effect on the pseudo numbers.
An example is the patch at https://patchwork.kernel.org/patch/9679745/
which avoid to create a bunch of phi-nodes and phi-sources that will
be simplified away (in fact not even used at all). After simplification
most code is exactly identical with or without this patch, but for the
pseudo numbers. And it this situation my patch will 'normalize' them
while yours won't (if I understand it correctly).

>> Well, it's true for now that we could reset the counter at each function
>> and this would already 'solve' most of the cases I had. But what if
>> we begin to add a second pass of automatic inlining?
>
> That is still fine, each pseudo has its own memory allocation.
> The number is just for show_instruction text output. Make sure
> reset the pseudo_base inside show_entry() will force the inlined function
> allocate new pseudo for different pseudo, base on the memory allocation.
> If after inline two different pseudo has the same "nr", as long as they
> both smaller than pseudo_base, they will be assign to different number
>  in the new round.
>
>> And keeping unique names every pseudo has advantages.
>
> I think each pseudo do has unique pointer and storage so that did not change.
> The "nr" is only for showing the text information.
>
> If we want to be smart about comparing the output, we should have
> a smarter diff against instructions. Right now if one of the basic block
> use one more pseudo will likely impact other basic block because the
> pseudo has shifted for other basic block, even it is the instruction code
> is equivalent for other basic block.

Yes, of course, just reseting the counter at each function entry would already
eliminate most differences because it forces any possible differences to
to stay local to the function and propagate to others.
To would be a trivial changes if we don't mind for the pseudo-numbers to
no more being unique within the file. I would gladly send a patch for it.

But as I explained here above this wouldn't cover all my cases and I think
that your proposal also won't.

And since the 'problem' being solved here is just some annoyance during
development (and avoiding, after some changes, to have to update some
unrelated tests), I think that this brute-force post-processing step only
used on demand by test-linearize, keeping the normal code simple as it is,
is the best thing to do.

And about a smarter diff, yes of course. When I'm doing batch comparison
of the output of test-linearize on 5500+ files, I often use a crude pre-diff
processing step with a sed script that simply and purely strip off all
pseudo-numbers! But maybe that should not be called a smarter diff but a
dumber one :)

-- Luc

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

* Re: [PATCH 3/3] allow to normalize the pseudos
  2017-06-05 22:10         ` Luc Van Oostenryck
@ 2017-06-06  2:42           ` Christopher Li
  0 siblings, 0 replies; 9+ messages in thread
From: Christopher Li @ 2017-06-06  2:42 UTC (permalink / raw)
  To: Luc Van Oostenryck; +Cc: Linux-Sparse

On Mon, Jun 5, 2017 at 3:10 PM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>> You will still need to reset the pseudo_base per show_entry().
>> That way, each function is reset the displaying pseudo to start
>> from 1. Same code should have the same pseudo number at the
>> same instruction, no?
>>
>> As far as I can tell, behavior should be very  similar to your patch.
>> Just don't need to do a separate pass to go through each instruction.
>
> It would fro the simple/normal cases but not in general because simplify
> away an instruction and not creating this instruction at first don't have the
> same effect on the pseudo numbers.

If the instruction has been delete, the pseudo number for that instruction
target pseudo will be reassign to the next instruction. I think that
is justifiable.
You remove instructions you get different numbers.

> An example is the patch at https://patchwork.kernel.org/patch/9679745/
> which avoid to create a bunch of phi-nodes and phi-sources that will
> be simplified away (in fact not even used at all). After simplification
> most code is exactly identical with or without this patch, but for the
> pseudo numbers. And it this situation my patch will 'normalize' them
> while yours won't (if I understand it correctly).

If we do reset pseudo_base at show_entry(), I think in this example
the loop-linearization.c output should be the same. The diff in
loop-linearization.c did not show any instruction actually get deleted.
In my suggestion the number is driven by show_pseudo() usage.
If the instruction did not show up in the diff, it will have no impact
in the numbering. As far as I can tell that output should be the same.

>
> Yes, of course, just reseting the counter at each function entry would already
> eliminate most differences because it forces any possible differences to
> to stay local to the function and propagate to others.
>
> To would be a trivial changes if we don't mind for the pseudo-numbers to
> no more being unique within the file. I would gladly send a patch for it.

The text output is only there for human to read. I think localization of the
pseudo number is actually good thing. If needed, I don't mind adding
a pseudo_base into struct entrypoint so we don't have to reset them
if we haven't modify instruction in the function.

> But as I explained here above this wouldn't cover all my cases and I think
> that your proposal also won't.

That I haven't understand why it won't work. Maybe I miss some thing
obvious.

>
> And since the 'problem' being solved here is just some annoyance during
> development (and avoiding, after some changes, to have to update some
> unrelated tests), I think that this brute-force post-processing step only
> used on demand by test-linearize, keeping the normal code simple as it is,
> is the best thing to do.

I was hoping for if the simple way can work then we don't need the brute
force one. Even the brute force one will suffer from change in one basic
block will impact the numbering of other basic block.

>
> And about a smarter diff, yes of course. When I'm doing batch comparison
> of the output of test-linearize on 5500+ files, I often use a crude pre-diff
> processing step with a sed script that simply and purely strip off all
> pseudo-numbers! But maybe that should not be called a smarter diff but a
> dumber one :)

That is why I am actually tempting to rewrite test-suite using python so it
is much easier to do text manipulation and compare.

Chris

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

end of thread, other threads:[~2017-06-06  2:42 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-03  7:34 [PATCH 0/3] normalize pseudos numbering Luc Van Oostenryck
2017-06-03  7:34 ` [PATCH 1/3] rename 'struct warning' to 'struct flag' Luc Van Oostenryck
2017-06-03  7:34 ` [PATCH 2/3] let handle_simple_switch() handle an array of flags Luc Van Oostenryck
2017-06-03  7:34 ` [PATCH 3/3] allow to normalize the pseudos Luc Van Oostenryck
2017-06-05 18:45   ` Christopher Li
2017-06-05 19:54     ` Luc Van Oostenryck
2017-06-05 20:50       ` Christopher Li
2017-06-05 22:10         ` Luc Van Oostenryck
2017-06-06  2:42           ` 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.