| Message ID | 20180318135531.10834-1-luc.vanoostenryck@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Sun, Mar 18, 2018 at 6:55 AM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > The function dominates() is used to test the dominance of > one instruction over another one. To do this different things > need to be checked, like: > - same symbol or not > - same address & same offset > - partial overlapping > > One case is not covered though: when type cohercion is done > via a pointer to an union (IOW when a memory location is used > to store one type and is then read back as another type). > For example, the location is first stored as a float and then is > read back as an integer of the same size. Currently, sparse > will consider that the store effectively dominates the load > which will then allow to simplify away the load and use the > pseudo holding the float value for the expected integer. > > There is surely several ways to fix this problem. > The solution used in this patch is to check if both instructions > are of of the same floating-pointness. > > Note: this solution is probably incomplete. For example, what > about compound types? Thanks for the patch. The test case is definitly good to have. The solution looks likely work for your test case. The more general topic is pointer alias analyse. 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
On Mon, Mar 19, 2018 at 02:19:51AM -0700, Christopher Li wrote: > On Sun, Mar 18, 2018 at 6:55 AM, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: > > The function dominates() is used to test the dominance of > > one instruction over another one. To do this different things > > need to be checked, like: > > - same symbol or not > > - same address & same offset > > - partial overlapping > > > > One case is not covered though: when type cohercion is done > > via a pointer to an union (IOW when a memory location is used > > to store one type and is then read back as another type). > > For example, the location is first stored as a float and then is > > read back as an integer of the same size. Currently, sparse > > will consider that the store effectively dominates the load > > which will then allow to simplify away the load and use the > > pseudo holding the float value for the expected integer. ... > > Note: this solution is probably incomplete. For example, what > > about compound types? > > Thanks for the patch. The test case is definitly good to have. > The solution looks likely work for your test case. > > The more general topic is pointer alias analyse. But AA is irrelevant to the problem here. AA will tell you *if* two pointers may or must alias each other. This is needed for a number of things but the problem here is different: we already *know* that (or only concerned when) the two locations *are* the same (if there is a doubt about this dominates() returns -1) but still we must not conclude that the store dominates the load. In fact, we can conclude that it dominates if we want so, but what must not be done is the corresponding memop simplification. The patch cover the case 'floating-point against anything else' and my question was along the line: What if we have more complex situations with *same* location but mixed types? What are the memop simplifications that may be done and which must not be done? For example what if we have an union of union or an union holding a struct holding an union of a int & a float (all of the same size)? I have the feeling that this test should move where the memop simplification is done. I'll see. -- 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
diff --git a/flow.c b/flow.c index f928c2684..32e30670c 100644 --- a/flow.c +++ b/flow.c @@ -367,6 +367,10 @@ int dominates(pseudo_t pseudo, struct instruction *insn, struct instruction *dom return 0; return -1; } + if (!insn->type || !dom->type) + return -1; + if (is_float_type(insn->type) != is_float_type(dom->type)) + return -1; return 1; } diff --git a/validation/mem2reg/access-union0.c b/validation/mem2reg/access-union0.c new file mode 100644 index 000000000..b04326cea --- /dev/null +++ b/validation/mem2reg/access-union0.c @@ -0,0 +1,37 @@ +typedef unsigned int u32; +typedef unsigned long u64; + +void use(u32); + +u32 f0(void) +{ + union { + u64 b; + u32 a[2]; + } u; + + u.b = 1; + return u.a[0]; +} + + +u32 f1(void) +{ + union { + u64 b; + u32 a[2]; + } u; + + u.b = 1; + return u.a[1]; +} + +/* + * check-name: access-union0 + * check-command: test-linearize -m64 -Wno-decl $file + * + * check-output-ignore + * check-output-pattern(2): load\\. + * check-output-pattern(2): store\\. + * check-output-excludes: ret.32 *\\$1 + */ diff --git a/validation/mem2reg/access-union1.c b/validation/mem2reg/access-union1.c new file mode 100644 index 000000000..e6d8c820e --- /dev/null +++ b/validation/mem2reg/access-union1.c @@ -0,0 +1,36 @@ +void use(int); + +double foo(void) +{ + union { + double d; + int i; + } u; + + u.d = 1.0; + use(u.i); + u.d = 0.0; + return u.d; +} + +/* + * check-name: access-union1 + * check-command: test-linearize -Wno-decl $file + * check-known-to-fail + * + * check-output-start +foo: +.L0: + <entry-point> + setfval.64 %r1 <- 1.000000 + store.64 %r1 <- 0[u] + load.32 %r2 <- 0[u] + call use, %r2 + setfval.64 %r3 <- 0.000000 + ret.64 %r3 + + + * check-output-end + * check-output-pattern(1): load\\. + * check-output-pattern(1): store\\. + */ diff --git a/validation/mem2reg/access-union2.c b/validation/mem2reg/access-union2.c new file mode 100644 index 000000000..7c119ffbd --- /dev/null +++ b/validation/mem2reg/access-union2.c @@ -0,0 +1,26 @@ +typedef double dbl; +typedef unsigned long u64; + +void use(u64); + +u64 foo(void) +{ + union { + dbl f; + u64 i; + } u; + + u.f = 1.0; + use(u.i); + u.f = 0.0; + return u.f; +} + +/* + * check-name: access-union2 + * check-command: test-linearize -Wno-decl $file + * + * check-output-ignore + * check-output-pattern(1): load\\. + * check-output-pattern(1): store\\. + */ diff --git a/validation/mem2reg/access-union3.c b/validation/mem2reg/access-union3.c new file mode 100644 index 000000000..d977493ab --- /dev/null +++ b/validation/mem2reg/access-union3.c @@ -0,0 +1,24 @@ +typedef double dbl; +typedef unsigned long u64; + +void use(u64); + +static dbl foo(void) +{ + union { + dbl f; + u64 i; + } u; + + u.i = 123; + return u.f; +} + +/* + * check-name: access-union3 + * check-command: test-linearize -fdump-ir=mem2reg $file + * + * check-output-ignore + * check-output-pattern(1): load\\. + * check-output-pattern(1): store\\. + */ diff --git a/validation/mem2reg/init-local.c b/validation/mem2reg/init-local-float.c similarity index 53% rename from validation/mem2reg/init-local.c rename to validation/mem2reg/init-local-float.c index d51c9247a..2642a204a 100644 --- a/validation/mem2reg/init-local.c +++ b/validation/mem2reg/init-local-float.c @@ -1,14 +1,4 @@ -int ssimple(void) -{ - struct { - int a; - } s; - - s.a = 1; - return s.a; -} - -double sdouble(void) +static double local_float(void) { struct { double a; @@ -20,7 +10,7 @@ double sdouble(void) /* * check-name: init-local - * check-command: test-linearize -Wno-decl -fdump-ir=mem2reg $file + * check-command: test-linearize $file * check-output-ignore * check-output-excludes: load\\. * check-output-excludes: store\\. diff --git a/validation/mem2reg/init-local-int.c b/validation/mem2reg/init-local-int.c new file mode 100644 index 000000000..285f3199f --- /dev/null +++ b/validation/mem2reg/init-local-int.c @@ -0,0 +1,18 @@ +static int local_int(void) +{ + struct { + int a; + } s; + + s.a = 1; + return s.a; +} + +/* + * check-name: init-local-int + * check-command: test-linearize $file + * check-output-ignore + * check-output-excludes: load\\. + * check-output-excludes: store\\. + * check-output-contains: ret.32 *\\$1 + */ diff --git a/validation/mem2reg/init-local-union0.c b/validation/mem2reg/init-local-union0.c index 3a57e781f..9a3b87d46 100644 --- a/validation/mem2reg/init-local-union0.c +++ b/validation/mem2reg/init-local-union0.c @@ -15,4 +15,5 @@ double uintfloat(void) * check-output-ignore * check-output-pattern(1): store\\.32 * check-output-pattern(1): load\\.64 + * check-output-excludes: ret\\.*\\$1 */
The function dominates() is used to test the dominance of one instruction over another one. To do this different things need to be checked, like: - same symbol or not - same address & same offset - partial overlapping One case is not covered though: when type cohercion is done via a pointer to an union (IOW when a memory location is used to store one type and is then read back as another type). For example, the location is first stored as a float and then is read back as an integer of the same size. Currently, sparse will consider that the store effectively dominates the load which will then allow to simplify away the load and use the pseudo holding the float value for the expected integer. There is surely several ways to fix this problem. The solution used in this patch is to check if both instructions are of of the same floating-pointness. Note: this solution is probably incomplete. For example, what about compound types? Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> --- flow.c | 4 +++ validation/mem2reg/access-union0.c | 37 ++++++++++++++++++++++ validation/mem2reg/access-union1.c | 36 +++++++++++++++++++++ validation/mem2reg/access-union2.c | 26 +++++++++++++++ validation/mem2reg/access-union3.c | 24 ++++++++++++++ .../mem2reg/{init-local.c => init-local-float.c} | 14 ++------ validation/mem2reg/init-local-int.c | 18 +++++++++++ validation/mem2reg/init-local-union0.c | 1 + 8 files changed, 148 insertions(+), 12 deletions(-) create mode 100644 validation/mem2reg/access-union0.c create mode 100644 validation/mem2reg/access-union1.c create mode 100644 validation/mem2reg/access-union2.c create mode 100644 validation/mem2reg/access-union3.c rename validation/mem2reg/{init-local.c => init-local-float.c} (53%) create mode 100644 validation/mem2reg/init-local-int.c