diff mbox series

[PR,90939] Remove outdated assert in ipcp_bits_lattice::meet_with

Message ID ri6ef3jkkq3.fsf@suse.cz
State New
Headers show
Series [PR,90939] Remove outdated assert in ipcp_bits_lattice::meet_with | expand

Commit Message

Martin Jambor June 24, 2019, 4:32 p.m. UTC 
Hi,

in August 2016 Prathamesh implemented inter-procedural propagation of
known non-zero bits on integers.  In August that same year he then also
added the ability to track it for pointer, replacing separate alignment
tracking.

However, we still have an assert in ipcp_bits_lattice::meet_with from
the first commit that checks that any binary operation coming from an
arithmetic jump function is performed on integers.  Martin discovered
that when you compile chromium and stars are aligned correctly, you can
end up evaluating a pointer expression MAX_EXPR (param, 0) there which
trips the assert.

Unless Prathamesh can remember a reason why the assert is important, I
believe the correct thing is just to remove it.  In the case of this
MAX_EXPR, it will end up being evaluated in bit_value_binop which cannot
handle it and so we will end up with a BOTTOM lattice in the end.  In
the general case, bit_value_binop operates on widest_ints and so shoud
have no problem dealing with pointers.

I'm bootstrapping and testing the following patch to satisfy the rules
but since it only removes an assert and adds a testcase that I checked,
I do not expect any problems.  Is it OK for trunk, GCC 9 and 8?

Thanks,

Martin




2019-06-24  Martin Jambor  <mjambor@suse.cz>

	PR ipa/90939
	* ipa-cp.c (ipcp_bits_lattice::meet_with): Remove assert.

	testsuite/
	* g++.dg/lto/pr90939_[01].C: New test.
---
 gcc/ipa-cp.c                         |  1 -
 gcc/testsuite/g++.dg/lto/pr90939_0.C | 64 ++++++++++++++++++++++++++++
 gcc/testsuite/g++.dg/lto/pr90939_1.C | 45 +++++++++++++++++++
 3 files changed, 109 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/lto/pr90939_0.C
 create mode 100644 gcc/testsuite/g++.dg/lto/pr90939_1.C

Comments

Prathamesh Kulkarni June 25, 2019, 7:36 a.m. UTC | #1 
On Mon, 24 Jun 2019 at 22:02, Martin Jambor <mjambor@suse.cz> wrote:
>
> Hi,
>
> in August 2016 Prathamesh implemented inter-procedural propagation of
> known non-zero bits on integers.  In August that same year he then also
> added the ability to track it for pointer, replacing separate alignment
> tracking.
>
> However, we still have an assert in ipcp_bits_lattice::meet_with from
> the first commit that checks that any binary operation coming from an
> arithmetic jump function is performed on integers.  Martin discovered
> that when you compile chromium and stars are aligned correctly, you can
> end up evaluating a pointer expression MAX_EXPR (param, 0) there which
> trips the assert.
>
> Unless Prathamesh can remember a reason why the assert is important, I
> believe the correct thing is just to remove it.  In the case of this
> MAX_EXPR, it will end up being evaluated in bit_value_binop which cannot
> handle it and so we will end up with a BOTTOM lattice in the end.  In
> the general case, bit_value_binop operates on widest_ints and so shoud
> have no problem dealing with pointers.
Hi,
I think the assert should have been removed after adding pointer
alignment propagation.
Sorry about that -:(

Thanks,
Prathamesh
>
> I'm bootstrapping and testing the following patch to satisfy the rules
> but since it only removes an assert and adds a testcase that I checked,
> I do not expect any problems.  Is it OK for trunk, GCC 9 and 8?
>
> Thanks,
>
> Martin
>
>
>
>
> 2019-06-24  Martin Jambor  <mjambor@suse.cz>
>
>         PR ipa/90939
>         * ipa-cp.c (ipcp_bits_lattice::meet_with): Remove assert.
>
>         testsuite/
>         * g++.dg/lto/pr90939_[01].C: New test.
> ---
>  gcc/ipa-cp.c                         |  1 -
>  gcc/testsuite/g++.dg/lto/pr90939_0.C | 64 ++++++++++++++++++++++++++++
>  gcc/testsuite/g++.dg/lto/pr90939_1.C | 45 +++++++++++++++++++
>  3 files changed, 109 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/g++.dg/lto/pr90939_0.C
>  create mode 100644 gcc/testsuite/g++.dg/lto/pr90939_1.C
>
> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> index d3a88756a91..69c00a9c5a5 100644
> --- a/gcc/ipa-cp.c
> +++ b/gcc/ipa-cp.c
> @@ -1085,7 +1085,6 @@ ipcp_bits_lattice::meet_with (ipcp_bits_lattice& other, unsigned precision,
>    if (TREE_CODE_CLASS (code) == tcc_binary)
>      {
>        tree type = TREE_TYPE (operand);
> -      gcc_assert (INTEGRAL_TYPE_P (type));
>        widest_int o_value, o_mask;
>        get_value_and_mask (operand, &o_value, &o_mask);
>
> diff --git a/gcc/testsuite/g++.dg/lto/pr90939_0.C b/gcc/testsuite/g++.dg/lto/pr90939_0.C
> new file mode 100644
> index 00000000000..8987c348015
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/lto/pr90939_0.C
> @@ -0,0 +1,64 @@
> +// PR ipa/90939
> +// { dg-lto-do link }
> +// { dg-lto-options { { -flto -O3 } } }
> +
> +
> +typedef char uint8_t;
> +template <class T> class A {
> +public:
> +  A(T *);
> +};
> +template <typename Derived, typename Base> const Derived &To(Base &p1) {
> +  return static_cast<const Derived &>(p1);
> +}
> +class H;
> +template <typename, typename Base> const H *To(Base *p1) {
> +  return p1 ? &To<H>(*p1) : nullptr;
> +}
> +enum TextDirection : uint8_t;
> +enum WritingMode : unsigned;
> +class B {
> +public:
> +  WritingMode m_fn1();
> +};
> +class C {
> +public:
> +  int &m_fn2();
> +};
> +class D { double d;};
> +class H : public D {};
> +class F {
> +public:
> +  F(C, A<const int>, B *, WritingMode, TextDirection);
> +};
> +
> +class G {
> +public:
> +  C NGLayoutAlgorithm_node;
> +  B NGLayoutAlgorithm_space;
> +  TextDirection NGLayoutAlgorithm_direction;
> +  H NGLayoutAlgorithm_break_token;
> +  G(A<const int> p1) __attribute__((noinline))
> +    : break_token_(&NGLayoutAlgorithm_break_token),
> +        container_builder_(NGLayoutAlgorithm_node, p1, &NGLayoutAlgorithm_space,
> +                           NGLayoutAlgorithm_space.m_fn1(),
> +                           NGLayoutAlgorithm_direction) {}
> +  G(C p1, const H *) : G(&p1.m_fn2()) {}
> +  A<H> break_token_;
> +  F container_builder_;
> +};
> +
> +class I : G {
> +public:
> +  I(const D *) __attribute__((noinline));
> +};
> +C a;
> +I::I(const D *p1) : G(a, To<H>(p1)) {}
> +
> +D gd[10];
> +
> +int main (int argc, char *argv[])
> +{
> +  I i(&(gd[argc%2]));
> +  return 0;
> +}
> diff --git a/gcc/testsuite/g++.dg/lto/pr90939_1.C b/gcc/testsuite/g++.dg/lto/pr90939_1.C
> new file mode 100644
> index 00000000000..9add89494d7
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/lto/pr90939_1.C
> @@ -0,0 +1,45 @@
> +typedef char uint8_t;
> +template <class T> class A {
> +public:
> +  A(T *);
> +};
> +
> +enum TextDirection : uint8_t;
> +enum WritingMode : unsigned;
> +class B {
> +public:
> +  WritingMode m_fn1();
> +};
> +class C {
> +public:
> +  int &m_fn2();
> +};
> +
> +class F {
> +public:
> +  F(C, A<const int>, B *, WritingMode, TextDirection);
> +};
> +class D { double d;};
> +class H : public D {};
> +
> +
> +
> +template <class T> A<T>::A(T*) {}
> +
> +template class A<H>;
> +template class A<int const>;
> +
> +WritingMode __attribute__((noipa))
> +B::m_fn1()
> +{
> +  return (WritingMode) 0;
> +}
> +
> +int gi;
> +int & __attribute__((noipa))
> +C::m_fn2 ()
> +{
> +  return gi;
> +}
> +
> +__attribute__((noipa)) F::F(C, A<const int>, B *, WritingMode, TextDirection) {}
> --
> 2.21.0
>
diff mbox series

Patch

diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index d3a88756a91..69c00a9c5a5 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -1085,7 +1085,6 @@  ipcp_bits_lattice::meet_with (ipcp_bits_lattice& other, unsigned precision,
   if (TREE_CODE_CLASS (code) == tcc_binary)
     {
       tree type = TREE_TYPE (operand);
-      gcc_assert (INTEGRAL_TYPE_P (type));
       widest_int o_value, o_mask;
       get_value_and_mask (operand, &o_value, &o_mask);
 
diff --git a/gcc/testsuite/g++.dg/lto/pr90939_0.C b/gcc/testsuite/g++.dg/lto/pr90939_0.C
new file mode 100644
index 00000000000..8987c348015
--- /dev/null
+++ b/gcc/testsuite/g++.dg/lto/pr90939_0.C
@@ -0,0 +1,64 @@ 
+// PR ipa/90939
+// { dg-lto-do link }
+// { dg-lto-options { { -flto -O3 } } }
+
+
+typedef char uint8_t;
+template <class T> class A {
+public:
+  A(T *);
+};
+template <typename Derived, typename Base> const Derived &To(Base &p1) {
+  return static_cast<const Derived &>(p1);
+}
+class H;
+template <typename, typename Base> const H *To(Base *p1) {
+  return p1 ? &To<H>(*p1) : nullptr;
+}
+enum TextDirection : uint8_t;
+enum WritingMode : unsigned;
+class B {
+public:
+  WritingMode m_fn1();
+};
+class C {
+public:
+  int &m_fn2();
+};
+class D { double d;};
+class H : public D {};
+class F {
+public:
+  F(C, A<const int>, B *, WritingMode, TextDirection);
+};
+
+class G {
+public:
+  C NGLayoutAlgorithm_node;
+  B NGLayoutAlgorithm_space;
+  TextDirection NGLayoutAlgorithm_direction;
+  H NGLayoutAlgorithm_break_token;
+  G(A<const int> p1) __attribute__((noinline))
+    : break_token_(&NGLayoutAlgorithm_break_token),
+        container_builder_(NGLayoutAlgorithm_node, p1, &NGLayoutAlgorithm_space,
+                           NGLayoutAlgorithm_space.m_fn1(),
+                           NGLayoutAlgorithm_direction) {}
+  G(C p1, const H *) : G(&p1.m_fn2()) {}
+  A<H> break_token_;
+  F container_builder_;
+};
+
+class I : G {
+public:
+  I(const D *) __attribute__((noinline));
+};
+C a;
+I::I(const D *p1) : G(a, To<H>(p1)) {}
+
+D gd[10];
+
+int main (int argc, char *argv[])
+{
+  I i(&(gd[argc%2]));
+  return 0;
+}
diff --git a/gcc/testsuite/g++.dg/lto/pr90939_1.C b/gcc/testsuite/g++.dg/lto/pr90939_1.C
new file mode 100644
index 00000000000..9add89494d7
--- /dev/null
+++ b/gcc/testsuite/g++.dg/lto/pr90939_1.C
@@ -0,0 +1,45 @@ 
+typedef char uint8_t;
+template <class T> class A {
+public:
+  A(T *);
+};
+
+enum TextDirection : uint8_t;
+enum WritingMode : unsigned;
+class B {
+public:
+  WritingMode m_fn1();
+};
+class C {
+public:
+  int &m_fn2();
+};
+
+class F {
+public:
+  F(C, A<const int>, B *, WritingMode, TextDirection);
+};
+class D { double d;};
+class H : public D {};
+
+
+
+template <class T> A<T>::A(T*) {}
+
+template class A<H>;
+template class A<int const>;
+
+WritingMode __attribute__((noipa))
+B::m_fn1()
+{
+  return (WritingMode) 0;
+}
+
+int gi;
+int & __attribute__((noipa))
+C::m_fn2 ()
+{
+  return gi;
+}
+
+__attribute__((noipa)) F::F(C, A<const int>, B *, WritingMode, TextDirection) {}