[PATCH] Add coalesce coverage test

11 views
Skip to first unread message

alas....@gmail.com

unread,
Jul 26, 2021, 9:12:59 AMJul 26
to isl-dev...@googlegroups.com, Andrei Lascu
From: Andrei Lascu <andrei....@imperial.ac.uk>

A new isl_test routine which achieves additional coverage. Of particular
interest, it covers the case where a pair of two maps to be fused are
rational basic maps (current lines 535-537 in `isl_coalesce.c`).

Signed-off-by: Andrei Lascu <andrei....@imperial.ac.uk>
---
isl_test.c | 32 +++++++++++++++++++++++++++++++-
1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/isl_test.c b/isl_test.c
index 2d096509..c2e8ff40 100644
--- a/isl_test.c
+++ b/isl_test.c
@@ -2585,6 +2585,35 @@ static isl_stat test_coalesce_special7(isl_ctx *ctx)
return test_coalesce_union(ctx, str1, str2);
}

+/* A test to provide code coverage of the case when fusing a pair of rational
+ * basic maps. In commit 54aac5ac, this represents lines 535-537 of the
+ * isl_coalesce.c source.
+ */
+static isl_stat test_coalesce_fuse_basic_map_rational(isl_ctx *ctx)
+{
+ const char *str_in, *str_out;
+ isl_set *s, *s_coalesced;
+ isl_basic_set *bs;
+ int equal;
+
+ str_in = "{ [-28, 2, 40]; [-21, 2, 30]; [-14, 2, 20]; [-14, 1, 20]; "
+ "[-7, 1, 10] }";
+ str_out = "{ [i0, i1, i2] : 7i2 = -10i0 and 0 < i1 <= 2 and "
+ "7i1 <= -i0 and 14i1 >= -i0 }";
+
+ s = isl_set_read_from_str(ctx, str_in);
+ s_coalesced = isl_set_coalesce(isl_set_copy(s));
+ s = isl_set_union(s, s_coalesced);
+ bs = isl_set_convex_hull(s);
+ equal = !strcmp(isl_basic_set_to_str(bs), str_out);
+
+ isl_basic_set_free(bs);
+ if (!equal)
+ isl_die(ctx, isl_error_unknown, "unexpected result", return -1);
+
+ return 0;
+}
+
/* Test the functionality of isl_set_coalesce.
* That is, check that the output is always equal to the input
* and in some cases that the result consists of a single disjunct.
@@ -2616,7 +2645,8 @@ static int test_coalesce(struct isl_ctx *ctx)
return -1;
if (test_coalesce_special7(ctx) < 0)
return -1;
-
+ if (test_coalesce_fuse_basic_map_rational(ctx) < 0)
+ return -1;

return 0;
}
--
2.32.0

Sven Verdoolaege

unread,
Jul 26, 2021, 3:09:20 PMJul 26
to alas....@gmail.com, isl-dev...@googlegroups.com, Andrei Lascu
On Mon, Jul 26, 2021 at 02:10:43PM +0100, alas....@gmail.com wrote:
> From: Andrei Lascu <andrei....@imperial.ac.uk>
>
> A new isl_test routine which achieves additional coverage. Of particular
> interest, it covers the case where a pair of two maps to be fused are
> rational basic maps (current lines 535-537 in `isl_coalesce.c`).
>
> Signed-off-by: Andrei Lascu <andrei....@imperial.ac.uk>
> ---
> isl_test.c | 32 +++++++++++++++++++++++++++++++-
> 1 file changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/isl_test.c b/isl_test.c
> index 2d096509..c2e8ff40 100644
> --- a/isl_test.c
> +++ b/isl_test.c
> @@ -2585,6 +2585,35 @@ static isl_stat test_coalesce_special7(isl_ctx *ctx)
> return test_coalesce_union(ctx, str1, str2);
> }
>
> +/* A test to provide code coverage of the case when fusing a pair of rational
> + * basic maps. In commit 54aac5ac, this represents lines 535-537 of the
> + * isl_coalesce.c source.

I'm not even sure these lines need to be mentioned in the commit message,
but they definitely shouldn't be metioned here. It's likely to go stale
pretty quickly.

> +static isl_stat test_coalesce_fuse_basic_map_rational(isl_ctx *ctx)
> +{
> + const char *str_in, *str_out;
> + isl_set *s, *s_coalesced;
> + isl_basic_set *bs;
> + int equal;
> +
> + str_in = "{ [-28, 2, 40]; [-21, 2, 30]; [-14, 2, 20]; [-14, 1, 20]; "
> + "[-7, 1, 10] }";
> + str_out = "{ [i0, i1, i2] : 7i2 = -10i0 and 0 < i1 <= 2 and "
> + "7i1 <= -i0 and 14i1 >= -i0 }";
> +
> + s = isl_set_read_from_str(ctx, str_in);
> + s_coalesced = isl_set_coalesce(isl_set_copy(s));
> + s = isl_set_union(s, s_coalesced);
> + bs = isl_set_convex_hull(s);

This is a pretty convoluted way of triggering those lines and
not guaranteed to trigger them in the future.
Why not a more direct test like "{ rat: [0:2]; rat: [1:3] }"?

> + equal = !strcmp(isl_basic_set_to_str(bs), str_out);

Don't compare string representations.
These are likely to change.

> + isl_basic_set_free(bs);
> + if (!equal)
> + isl_die(ctx, isl_error_unknown, "unexpected result", return -1);
> +
> + return 0;

Use isl_stat_ok and isl_stat_error instead.

skimo

Andrei Lascu

unread,
Jul 26, 2021, 3:24:32 PMJul 26
to isl Development
Thanks. Replies inline. Should I resend a fresh patch after the changes are done?
On Monday, July 26, 2021 at 8:09:20 PM UTC+1 sven.verdoolaege.isl wrote:
On Mon, Jul 26, 2021 at 02:10:43PM +0100, alas....@gmail.com wrote:
> From: Andrei Lascu <andrei....@imperial.ac.uk>
>
> A new isl_test routine which achieves additional coverage. Of particular
> interest, it covers the case where a pair of two maps to be fused are
> rational basic maps (current lines 535-537 in `isl_coalesce.c`).
>
> Signed-off-by: Andrei Lascu <andrei....@imperial.ac.uk>
> ---
> isl_test.c | 32 +++++++++++++++++++++++++++++++-
> 1 file changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/isl_test.c b/isl_test.c
> index 2d096509..c2e8ff40 100644
> --- a/isl_test.c
> +++ b/isl_test.c
> @@ -2585,6 +2585,35 @@ static isl_stat test_coalesce_special7(isl_ctx *ctx)
> return test_coalesce_union(ctx, str1, str2);
> }
>
> +/* A test to provide code coverage of the case when fusing a pair of rational
> + * basic maps. In commit 54aac5ac, this represents lines 535-537 of the
> + * isl_coalesce.c source.

I'm not even sure these lines need to be mentioned in the commit message,
but they definitely shouldn't be metioned here. It's likely to go stale
pretty quickly.
I agree, but unsure how to better express the achieved coverage (also was unsure if what I said about the pair of rational maps was even correct). Should I remove the mention of the commit/lines and leave nothing in place?

> +static isl_stat test_coalesce_fuse_basic_map_rational(isl_ctx *ctx)
> +{
> + const char *str_in, *str_out;
> + isl_set *s, *s_coalesced;
> + isl_basic_set *bs;
> + int equal;
> +
> + str_in = "{ [-28, 2, 40]; [-21, 2, 30]; [-14, 2, 20]; [-14, 1, 20]; "
> + "[-7, 1, 10] }";
> + str_out = "{ [i0, i1, i2] : 7i2 = -10i0 and 0 < i1 <= 2 and "
> + "7i1 <= -i0 and 14i1 >= -i0 }";
> +
> + s = isl_set_read_from_str(ctx, str_in);
> + s_coalesced = isl_set_coalesce(isl_set_copy(s));
> + s = isl_set_union(s, s_coalesced);
> + bs = isl_set_convex_hull(s);

This is a pretty convoluted way of triggering those lines and
not guaranteed to trigger them in the future.
Why not a more direct test like "{ rat: [0:2]; rat: [1:3] }"?
This is because for me isl is more of a gray, if not entirely black, box. I'll try what you propose and see if the coverage desired is still achieved.

> + equal = !strcmp(isl_basic_set_to_str(bs), str_out);

Don't compare string representations.
These are likely to change.
I'll replace with isl_set_is_equal.

> + isl_basic_set_free(bs);
> + if (!equal)
> + isl_die(ctx, isl_error_unknown, "unexpected result", return -1);
> +
> + return 0;

Use isl_stat_ok and isl_stat_error instead.
Alright.

Andrei

skimo

Sven Verdoolaege

unread,
Jul 26, 2021, 3:47:43 PMJul 26
to Andrei Lascu, isl Development
On Mon, Jul 26, 2021 at 12:24:32PM -0700, Andrei Lascu wrote:
> Thanks. Replies inline. Should I resend a fresh patch after the changes are
> done?

If you want it to go in as your change, yes.

> On Monday, July 26, 2021 at 8:09:20 PM UTC+1 sven.verdoolaege.isl wrote:
> > On Mon, Jul 26, 2021 at 02:10:43PM +0100, alas....@gmail.com wrote:
> > > From: Andrei Lascu <andrei....@imperial.ac.uk>
> > > +/* A test to provide code coverage of the case when fusing a pair of
> > rational
> > > + * basic maps. In commit 54aac5ac, this represents lines 535-537 of the
> > > + * isl_coalesce.c source.
> >
> > I'm not even sure these lines need to be mentioned in the commit message,
> > but they definitely shouldn't be metioned here. It's likely to go stale
> > pretty quickly.
> >
> I agree, but unsure how to better express the achieved coverage (also was
> unsure if what I said about the pair of rational maps was even correct).
> Should I remove the mention of the commit/lines and leave nothing in place?

I don't see what's so special about this test case that
it needs any further explanation.
I would just add a line to coalesce_tests and be done with it.

skimo

alas....@gmail.com

unread,
Jul 27, 2021, 10:31:59 AMJul 27
to isl-dev...@googlegroups.com, Andrei Lascu
From: Andrei Lascu <andrei....@imperial.ac.uk>

Added a new test to `convex_hull_tests[]` which triggers additional
coverage in `isl_coalesce.c`.

Signed-off-by: Andrei Lascu <andrei....@imperial.ac.uk>
---
As per Sven's suggestion above, I was able to greatly shrink the needed
code to trigger the discussed coverage. I can start a fresh thread if
needed, but as long as the test is integrated either way, I'm fine with
anything.

isl_test.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/isl_test.c b/isl_test.c
index 2d096509..914b928b 100644
--- a/isl_test.c
+++ b/isl_test.c
@@ -1638,6 +1638,8 @@ struct {
"[0, 2, 0]; [3, 1, 0] }",
"{ [a, b, c] : b > -a and 2b >= -1 + a and 0 <= c <= a and "
"5c <= 6 - a - 3b }" },
+ { "{ rat: [0:2]; rat: [1:3] }",
+ "{ [i0] : 0 <= i0 <= 3 }" },
};

static int test_convex_hull_algo(isl_ctx *ctx, int convex)
--
2.32.0

Sven Verdoolaege

unread,
Jul 27, 2021, 12:37:51 PMJul 27
to alas....@gmail.com, isl-dev...@googlegroups.com, Andrei Lascu
On Tue, Jul 27, 2021 at 03:31:17PM +0100, alas....@gmail.com wrote:
> From: Andrei Lascu <andrei....@imperial.ac.uk>
>
> Added a new test to `convex_hull_tests[]` which triggers additional
> coverage in `isl_coalesce.c`.

Use imperative or add a subject (and use present tense).
Also, you've now removed the description of what you want to cover
completely.

> Signed-off-by: Andrei Lascu <andrei....@imperial.ac.uk>
> ---
> As per Sven's suggestion above, I was able to greatly shrink the needed
> code to trigger the discussed coverage. I can start a fresh thread if
> needed, but as long as the test is integrated either way, I'm fine with
> anything.

I suggested you add a test to coalesce_tests,
but you added one to convex_hull_tests instead. Why?
If this is meant to increase coverage of the coalesce operation,
wouldn't it make more sense as a coalesce test?

skimo

Andrei Lascu

unread,
Jul 27, 2021, 2:37:10 PMJul 27
to isl Development
On Tuesday, July 27, 2021 at 5:37:51 PM UTC+1 sven.verdoolaege.isl wrote:
On Tue, Jul 27, 2021 at 03:31:17PM +0100, alas....@gmail.com wrote:
> From: Andrei Lascu <andrei....@imperial.ac.uk>
>
> Added a new test to `convex_hull_tests[]` which triggers additional
> coverage in `isl_coalesce.c`.

Use imperative or add a subject (and use present tense).
Also, you've now removed the description of what you want to cover
completely.
Will do for the next patch attempt. 


> Signed-off-by: Andrei Lascu <andrei....@imperial.ac.uk>
> ---
> As per Sven's suggestion above, I was able to greatly shrink the needed
> code to trigger the discussed coverage. I can start a fresh thread if
> needed, but as long as the test is integrated either way, I'm fine with
> anything.

I suggested you add a test to coalesce_tests,
but you added one to convex_hull_tests instead. Why?
If this is meant to increase coverage of the coalesce operation,
wouldn't it make more sense as a coalesce test?
Because apparently the sequence to trigger the coverage in `isl_coalesce.c` based on the input string you suggest is to read in the test, then perform a `convex_hull`, rather than a `coalesce`. I can of course add a test within the coalesce block of tests which calls convex hull, as it makes sense theoretically, but practically it felt better to add it to that array, as there is where it seems the test binary performs convex hulls in bulk. I've checked with calling `coalesce` and it does not trigger the extra coverage. I can provide a stack trace of how the coalesce coverage is achieved via the convex hull operation, if you think that's useful?

Andrei


skimo

Sven Verdoolaege

unread,
Jul 28, 2021, 4:22:06 PMJul 28
to Andrei Lascu, isl Development
On Tue, Jul 27, 2021 at 11:37:10AM -0700, Andrei Lascu wrote:
> bulk. I've checked with calling `coalesce` and it does not trigger the
> extra coverage. I can provide a stack trace of how the coalesce coverage is
> achieved via the convex hull operation, if you think that's useful?

It would be more useful if you could show that the patch
I just sent out does NOT trigger line 537 of isl_coalesce.c.
I'm pretty sure that it does, though.

skimo

Andrei Lascu

unread,
Jul 29, 2021, 9:23:27 AMJul 29
to isl Development
You are correct; I checked and it does. Unsure what I did that I wasn't able to trigger the coverage with the same input set. Thanks a lot for the help in getting this coverage right.

Andrei
Reply all
Reply to author
Forward
0 new messages