Bug 69336 - Constant value not detected
Summary: Constant value not detected
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 6.0
: P3 enhancement
Target Milestone: ---
Assignee: Richard Biener
URL:
Keywords: missed-optimization
: 69358 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-01-17 23:05 UTC by Marc Glisse
Modified: 2016-01-29 20:37 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Known to work: 6.0
Known to fail:
Last reconfirmed: 2016-01-17 00:00:00


Attachments
testcase (1.01 KB, text/x-csrc)
2016-01-17 23:05 UTC, Marc Glisse
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Glisse 2016-01-17 23:05:36 UTC
Created attachment 37384 [details]
testcase

This code was posted on the boost ML for unrelated reasons, I just commented out the last line. Compiling with -O3, I would expect the whole thing to compile to nothing, but instead I get in the .optimized dump the rather sad:

  cmap._values[1].second.second = "pear";
  _12 = MEM[(const char * const &)&cmap]._values[1].second.second;
  if (_12 == 0B)

I didn't check, it might be that just one more pass would get it, dom3 is quite busy on this example, but unless we intend to loop on passes, I believe we should have optimized this already in some earlier pass. I am sure there are already several PRs about similar issues, feel free to close this one if it seems too redundant.
Comment 1 Andrew Pinski 2016-01-17 23:45:04 UTC
There is another bug recorded somewhere about the cmap._values[1].second.second vs MEM[(const char * const &)&cmap]._values[1].second.second issue.
Comment 2 Richard Biener 2016-01-18 10:40:35 UTC
Yes, I think there is a duplicate (and a proposed patch).
Comment 3 Richard Biener 2016-01-18 12:50:25 UTC
Not fixed with r232508.  Fixed with the suggested

Index: gcc/tree-ssa-scopedtables.c
===================================================================
--- gcc/tree-ssa-scopedtables.c (revision 232508)
+++ gcc/tree-ssa-scopedtables.c (working copy)
@@ -214,7 +214,7 @@ avail_expr_hash (class expr_hash_elt *p)
     {
       /* T could potentially be a switch index or a goto dest.  */
       tree t = expr->ops.single.rhs;
-      if (TREE_CODE (t) == MEM_REF || TREE_CODE (t) == ARRAY_REF)
+      if (TREE_CODE (t) == MEM_REF || handled_component_p (t))
        {
          /* Make equivalent statements of both these kinds hash together.
             Dealing with both MEM_REF and ARRAY_REF allows us not to care
@@ -251,9 +251,9 @@ avail_expr_hash (class expr_hash_elt *p)
 static bool
 equal_mem_array_ref_p (tree t0, tree t1)
 {
-  if (TREE_CODE (t0) != MEM_REF && TREE_CODE (t0) != ARRAY_REF)
+  if (TREE_CODE (t0) != MEM_REF && ! handled_component_p (t0))
     return false;
-  if (TREE_CODE (t1) != MEM_REF && TREE_CODE (t1) != ARRAY_REF)
+  if (TREE_CODE (t1) != MEM_REF && ! handled_component_p (t1))
     return false;
 
   if (!types_compatible_p (TREE_TYPE (t0), TREE_TYPE (t1)))
Comment 4 Alan Lawrence 2016-01-18 14:49:36 UTC
That looks reasonable, AFAICT get_ref_base_and_extent will deal with anything that is handled_component_p. The same patch enables the optimization on aarch64, with appropriate --param sra-max-scalarization-size-Ospeed to pull the constant-pool entry in.
Comment 5 Richard Biener 2016-01-19 13:27:29 UTC
Fixed on trunk.
Comment 6 Richard Biener 2016-01-19 13:27:43 UTC
Author: rguenth
Date: Tue Jan 19 13:27:11 2016
New Revision: 232559

URL: https://gcc.gnu.org/viewcvs?rev=232559&root=gcc&view=rev
Log:
2016-01-19  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/69336
	* tree-ssa-scopedtables.c (avail_expr_hash): Handle all
	handled components with get_ref_base_and_extent.
	(equal_mem_array_ref_p): Adjust.

	* g++.dg/tree-ssa/pr69336.C: New testcase.

Added:
    trunk/gcc/testsuite/g++.dg/tree-ssa/pr69336.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssa-scopedtables.c
Comment 7 Jakub Jelinek 2016-01-19 13:51:02 UTC
*** Bug 69358 has been marked as a duplicate of this bug. ***
Comment 8 Jakub Jelinek 2016-01-19 23:10:04 UTC
*** Bug 69368 has been marked as a duplicate of this bug. ***
Comment 9 ktkachov 2016-01-22 18:24:07 UTC
(In reply to Richard Biener from comment #6)

> 
> 	* g++.dg/tree-ssa/pr69336.C: New testcase.

This testcase fails for me on arm-none-eabi.
The optimized dump on -std=c++14 -O3 -fdump-tree-optimized -std=c++14 contains:
int main() ()
{
  size_t n;
  const struct static_map cmap;
  const char * _4;
  int _5;
  int _11;
  void * _15;
  int _25;

  <bb 2>:
  cmap = *.LC3;
  _5 = cmap._values[0].second.first;
  if (_5 == 8)
    goto <bb 5>;
  else
    goto <bb 3>;

  <bb 3>:
  _25 = cmap._values[1].second.first;
  if (_25 == 8)
    goto <bb 5>;
  else
    goto <bb 4>;

  <bb 4>:
  _11 = cmap._values[2].second.first;
  if (_11 == 8)
    goto <bb 5>;
  else
    goto <bb 6>;

  <bb 5>:
  # n_8 = PHI <2(4), 0(2), 1(3)>
  _4 = MEM[(const char * const &)&cmap]._values[n_8].second.second;
  if (_4 == 0B)
    goto <bb 9>;
  else
    goto <bb 10>;

  <bb 6>:
  _15 = __cxa_allocate_exception (8);
  std::out_of_range::out_of_range (_15, "Key not found");

  <bb 7>:
  __cxa_throw (_15, &_ZTISt12out_of_range, __comp_dtor );

<L6>:
  __cxa_free_exception (_15);
  __builtin_cxa_end_cleanup ();

  <bb 9>:
  abort ();

  <bb 10>:
  cmap ={v} {CLOBBER};
  return 0;

}
Comment 10 Dominik Vogt 2016-01-27 06:51:44 UTC
The new test fails on s390x; what should I do about it?
(see https://gcc.gnu.org/ml/gcc-patches/2016-01/msg01359.html )
Comment 11 Richard Biener 2016-01-27 08:26:33 UTC
Should be fixed with

2016-01-25  Richard Biener  <rguenther@suse.de>

        PR testsuite/69380
        * g++.dg/tree-ssa/pr69336.C: Restrict to x86_64 and i?86.
Comment 12 Dominik Vogt 2016-01-27 09:58:41 UTC
The test works now on s390x.  Thanks.
Comment 13 Wilco 2016-01-29 19:09:02 UTC
(In reply to Richard Biener from comment #6)
> Author: rguenth
> Date: Tue Jan 19 13:27:11 2016
> New Revision: 232559
> 
> URL: https://gcc.gnu.org/viewcvs?rev=232559&root=gcc&view=rev

Even with this fix it still fails SPEC2006 gamess on AArch64 exactly as reported in bug 69368.  It passes if I revert the EXPR_SINGLE case in hashable_expr_equal_p to what it was before r232055 (ie. equal_mem_array_ref_p is still not correct).
Comment 14 rguenther@suse.de 2016-01-29 20:37:51 UTC
On January 29, 2016 8:09:02 PM GMT+01:00, wdijkstr at arm dot com <gcc-bugzilla@gcc.gnu.org> wrote:
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69336
>
>Wilco <wdijkstr at arm dot com> changed:
>
>           What    |Removed                     |Added
>----------------------------------------------------------------------------
>                CC|                            |wdijkstr at arm dot com
>
>--- Comment #13 from Wilco <wdijkstr at arm dot com> ---
>(In reply to Richard Biener from comment #6)
>> Author: rguenth
>> Date: Tue Jan 19 13:27:11 2016
>> New Revision: 232559
>> 
>> URL: https://gcc.gnu.org/viewcvs?rev=232559&root=gcc&view=rev
>
>Even with this fix it still fails SPEC2006 gamess on AArch64 exactly as
>reported in bug 69368.  It passes if I revert the EXPR_SINGLE case in
>hashable_expr_equal_p to what it was before r232055 (ie.
>equal_mem_array_ref_p
>is still not correct).

Can you re-open that bug please and try to track down the issue somewhat?