Bug 81741 - Misoptimisation : replacing a constant field read access by a function call
Summary: Misoptimisation : replacing a constant field read access by a function call
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 7.1.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: missed-optimization
Depends on:
Blocks:
 
Reported: 2017-08-06 16:21 UTC by Patrick Pelissier
Modified: 2017-09-13 17:20 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2017-08-08 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Pelissier 2017-08-06 16:21:00 UTC
GCC 7.1.0 generates for the following code:


#include <string.h>

typedef struct string_s {
  unsigned long size, alloc;
  char *ptr;
} string_t[1];

# define M_ASSUME(x)                                    \
  (! __builtin_constant_p (!!(x) || !(x)) || (x) ?      \
   (void) 0 : __builtin_unreachable())

int f(string_t s)
{
  M_ASSUME(strlen(s->ptr) == s->size);
  return s->size;
}


the following code on an x86-64 platform (gcc -std=c99 -O2 -S test.c):


f:
	subq	$8, %rsp
	movq	16(%rdi), %rdi
	call	strlen
	addq	$8, %rsp
	ret


Notice that the field access s->size is replaced by strlen(s->ptr), which is way slower.

GCC 4.9 doesn't have this issue.
Comment 1 Jakub Jelinek 2017-08-07 07:09:40 UTC
I see the same behavior with 4.9, as well as older (e.g. 4.8 or 4.4).
The thing is that __builtin_constant_p is (at -O1 and above) a check whether some expression is constant after optimizations, and here FRE or DOM passes figure out (due to jump threading?) that it is constant.
Abusing __builtin_constant_p this way is always going to be risky.
Comment 2 Patrick Pelissier 2017-08-07 19:47:53 UTC
I can reproduce the behavior without __builtin_constant_p by removing it from the M_ASSUME macro :

# define M_ASSUME(x)                                    \
  (  (x) ?						\
   (void) 0 : __builtin_unreachable())

It still generates the same instructions.
Comment 3 Richard Biener 2017-08-08 07:32:50 UTC
I'm quite sure it is because of propagating a conditional equivalence.

-fno-tree-dominator-opts fixes this (it's the only pass doing this kind of
propagation IIRC).

  <bb 2> [100.00%]:
  _1 = s_5(D)->ptr;
  _2 = strlen (_1);
  _3 = s_5(D)->size;
  if (_2 != _3)
    goto <bb 3>; [0.04%]
  else
    goto <bb 4>; [99.96%]

  <bb 3> [0.04%]:
  __builtin_unreachable ();

  <bb 4> [99.96%]:
  _6 = (int) _3;
  return _6;

here DOM decides well, on the else path _2 == _3 so let's propagate!

IMHO we _never_ should propagate (non-constant) conditional equivalences.
Comment 4 Jeffrey A. Law 2017-08-08 15:35:30 UTC
I've got changes to remove the propagation of conditional equivalences in DOM, but still allow us to use those equivalences for simplifications.  However, they're backed up behind several other things.  The most pressing of which is the stack-clash-protection work.

Review work on stack-clash-protection would be appreciated.

And FWIW, RTL CSE does conditional equivalence propagation as well.
Comment 5 Jeffrey A. Law 2017-08-22 15:13:42 UTC
Author: law
Date: Tue Aug 22 15:13:09 2017
New Revision: 251279

URL: https://gcc.gnu.org/viewcvs?rev=251279&root=gcc&view=rev
Log:
	PR tree-optimization/81741
	PR tree-optimization/71947
	* tree-ssa-dom.c: Include tree-inline.h.
	(record_temporary_equivalences): Only record SSA_NAME = SSA_NAME
	equivalences if one is more expensive to compute than the other.
	* tree-ssa-scopedtables.h (class const_or_copies): Make
	record_const_or_copy_raw method private.
	(class avail_exprs_stack): New method simplify_binary_operation.
	* tree-ssa-scopedtables.c (avail_exprs_stack::lookup_avail_expr): Call
	avail_exprs_stack::simplify_binary_operation as needed.
	(avail_exprs_stack::simplify_binary_operation): New function.

	PR tree-optimization/81741
	PR tree-optimization/71947
	* gcc.dg/tree-ssa/pr81741.c: New test.
	* gcc.dg/tree-ssa/pr71947-7.c: New test.
	* gcc.dg/tree-ssa/pr71947-8.c: New test.
	* gcc.dg/tree-ssa/pr71947-9.c: New test.
	* gcc.dg/tree-ssa/pr71941-1.c: Tweak expected output.
	* gcc.dg/tree-ssa/pr71941-2.c: Tweak expected output.
	* gcc.dg/tree-ssa/pr71941-3.c: Tweak expected output.
	* gcc.dg/tree-ssa/20030922-2.c: xfail.

Added:
    trunk/gcc/testsuite/gcc.dg/tree-ssa/pr71947-7.c
      - copied, changed from r251276, trunk/gcc/testsuite/gcc.dg/tree-ssa/pr71947-2.c
    trunk/gcc/testsuite/gcc.dg/tree-ssa/pr71947-8.c
      - copied, changed from r251276, trunk/gcc/testsuite/gcc.dg/tree-ssa/pr71947-2.c
    trunk/gcc/testsuite/gcc.dg/tree-ssa/pr71947-9.c
      - copied, changed from r251276, trunk/gcc/testsuite/gcc.dg/tree-ssa/pr71947-2.c
    trunk/gcc/testsuite/gcc.dg/tree-ssa/pr81741.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.dg/tree-ssa/20030922-2.c
    trunk/gcc/testsuite/gcc.dg/tree-ssa/pr71947-1.c
    trunk/gcc/testsuite/gcc.dg/tree-ssa/pr71947-2.c
    trunk/gcc/testsuite/gcc.dg/tree-ssa/pr71947-3.c
    trunk/gcc/tree-ssa-dom.c
    trunk/gcc/tree-ssa-scopedtables.c
    trunk/gcc/tree-ssa-scopedtables.h
Comment 6 Jeffrey A. Law 2017-08-22 15:14:08 UTC
Should be fixed on the trunk now.
Comment 7 Aldy Hernandez 2017-09-13 17:20:51 UTC
Author: aldyh
Date: Wed Sep 13 17:20:19 2017
New Revision: 252532

URL: https://gcc.gnu.org/viewcvs?rev=252532&root=gcc&view=rev
Log:
	PR tree-optimization/81741
	PR tree-optimization/71947
	* tree-ssa-dom.c: Include tree-inline.h.
	(record_temporary_equivalences): Only record SSA_NAME = SSA_NAME
	equivalences if one is more expensive to compute than the other.
	* tree-ssa-scopedtables.h (class const_or_copies): Make
	record_const_or_copy_raw method private.
	(class avail_exprs_stack): New method simplify_binary_operation.
	* tree-ssa-scopedtables.c (avail_exprs_stack::lookup_avail_expr): Call
	avail_exprs_stack::simplify_binary_operation as needed.
	(avail_exprs_stack::simplify_binary_operation): New function.

	PR tree-optimization/81741
	PR tree-optimization/71947
	* gcc.dg/tree-ssa/pr81741.c: New test.
	* gcc.dg/tree-ssa/pr71947-7.c: New test.
	* gcc.dg/tree-ssa/pr71947-8.c: New test.
	* gcc.dg/tree-ssa/pr71947-9.c: New test.
	* gcc.dg/tree-ssa/pr71941-1.c: Tweak expected output.
	* gcc.dg/tree-ssa/pr71941-2.c: Tweak expected output.
	* gcc.dg/tree-ssa/pr71941-3.c: Tweak expected output.
	* gcc.dg/tree-ssa/20030922-2.c: xfail.

Added:
    branches/range-gen2/gcc/testsuite/gcc.dg/tree-ssa/pr71947-7.c
      - copied, changed from r252531, branches/range-gen2/gcc/testsuite/gcc.dg/tree-ssa/pr71947-2.c
    branches/range-gen2/gcc/testsuite/gcc.dg/tree-ssa/pr71947-8.c
      - copied, changed from r252531, branches/range-gen2/gcc/testsuite/gcc.dg/tree-ssa/pr71947-2.c
    branches/range-gen2/gcc/testsuite/gcc.dg/tree-ssa/pr71947-9.c
      - copied, changed from r252531, branches/range-gen2/gcc/testsuite/gcc.dg/tree-ssa/pr71947-2.c
    branches/range-gen2/gcc/testsuite/gcc.dg/tree-ssa/pr81741.c
Modified:
    branches/range-gen2/gcc/ChangeLog
    branches/range-gen2/gcc/testsuite/ChangeLog
    branches/range-gen2/gcc/testsuite/gcc.dg/tree-ssa/20030922-2.c
    branches/range-gen2/gcc/testsuite/gcc.dg/tree-ssa/pr71947-1.c
    branches/range-gen2/gcc/testsuite/gcc.dg/tree-ssa/pr71947-2.c
    branches/range-gen2/gcc/testsuite/gcc.dg/tree-ssa/pr71947-3.c
    branches/range-gen2/gcc/tree-ssa-dom.c
    branches/range-gen2/gcc/tree-ssa-scopedtables.c
    branches/range-gen2/gcc/tree-ssa-scopedtables.h