Re: [PATCH 12/18] haifa-sched.c: make insn_queue[] a vec

[Bernd Schmidt]

On 04/20/2016 08:22 AM, tbsaunde+...@tbsaunde.org wrote:

-/* Remove INSN from queue.  */
+/* Remove INSN at idx from queue.  */
+static void
+queue_remove (unsigned int q, unsigned int idx)
+{
+  QUEUE_INDEX (insn_queue[q][idx]) = QUEUE_NOWHERE;
+  insn_queue[q].ordered_remove (idx);
+  q_size--;


I think I'm nacking this one, sorry. I don't think ordered_removes in 
the scheduler queues are going to fly.



Bernd

Re: [PATCH 09/18] make pattern_regs a vec

[Bernd Schmidt]

On 04/20/2016 08:22 AM, tbsaunde+...@tbsaunde.org wrote:


-static rtx_expr_list *
+static vec
  extract_mentioned_regs (rtx x)
  {
-  rtx_expr_list *mentioned_regs = NULL;
+  vec mentioned_regs = vNULL;
subrtx_var_iterator::array_type array;
FOR_EACH_SUBRTX_VAR (iter, array, x, NONCONST)
  {
rtx x = *iter;
if (REG_P (x))
-   mentioned_regs = alloc_EXPR_LIST (0, x, mentioned_regs);
+   mentioned_regs.safe_push (x);
  }
return mentioned_regs;
  }


Is it really such a great idea to return a vec by value? I'd rather pass 
a pointer to it into the function and operate on that.



Bernd

Re: [PATCH 15/18] make nonlocal_goto_handler_labels a vec

[Bernd Schmidt]

On 04/20/2016 08:22 AM, tbsaunde+...@tbsaunde.org wrote:

-  remove_node_from_insn_list (insn, _goto_handler_labels);
+
+  unsigned int len = vec_safe_length (nonlocal_goto_handler_labels);
+  for (unsigned int i = 0; i < len; i++)
+   if ((*nonlocal_goto_handler_labels)[i] == insn)
+ {
+   nonlocal_goto_handler_labels->ordered_remove (i);
+   break;
+ }


Why not unordered_remove?

Should there be a vec-based version of remove-node_from_*_list?


+  rtx_insn *temp;
+  unsigned int i;
+  FOR_EACH_VEC_SAFE_ELT_REVERSE (nonlocal_goto_handler_labels, i, temp)
+  BLOCK_FOR_INSN (temp)->flags |= BB_NON_LOCAL_GOTO_TARGET;


Indentation looks wrong.



/* Process non-local goto edges.  */
if (can_nonlocal_goto (insn))
-   for (rtx_insn_list *lab = nonlocal_goto_handler_labels;
-lab;
-lab = lab->next ())
- maybe_record_trace_start_abnormal (lab->insn (), insn);
+   {


Unnecessary brace?


/* Re-insert the EH_REGION notes.  */
-  if (eh_note || (was_call && nonlocal_goto_handler_labels))
+  if (eh_note || (was_call && vec_safe_length (nonlocal_goto_handler_labels)))


I'm not a big fan of omitting the > 0 and using the integer as a 
boolean. Multiple occurrences.



+  FOR_EACH_VEC_SAFE_ELT_REVERSE (nonlocal_goto_handler_labels, i, insn)
+  set_label_offsets (insn, NULL, 1);


Indentation.


Bernd

Re: [PATCH 00/18] towards removing rtx_insn_list and rtx_expr_list

[Bernd Schmidt]

On 04/21/2016 01:24 AM, Trevor Saunders wrote:

On Wed, Apr 20, 2016 at 06:03:01AM -0700, Andi Kleen wrote:



A vector can have very different performance than a list, depending how
it is used. Do your patches cause any measure performance difference for
the compiler?


I haven't measured, but I am aware of that and did consider it when
writing these patches.  I expect they'll help perf some since I went
through some hoops to not move elements around the vector unnecessarily.
I'm not really sure what work load is most effected by each of these
patches, and they don't really seem that risky to me so I'd rather notdo
tons of testing on the off chance they slow something down, in the worst
case we can always revert something to a list without using rtx.


Well, at least post before/after numbers (with all patches applied for 
the "after") for compiling a large source file, including time and 
memory reports.



Bernd

Re: [PATCH 07/18] loop-iv.c: make cond_list a vec

[Bernd Schmidt]

On 04/20/2016 08:22 AM, tbsaunde+...@tbsaunde.org wrote:

From: Trevor Saunders 



+ unsigned int len = cond_list.length ();
+ for (unsigned int i = len - 1; i < len; i--)


This is a really icky way to write a loop, the i < len condition makes 
it look like a forward one. We have FOR_EACH_VEC_ELT{,_REVERSE}, any 
reason not to use these?



Bernd

Re: An abridged "Writing C" for the gcc web pages

[Bernd Schmidt]

On 04/22/2016 09:45 PM, Sandra Loosemore wrote:

On 04/22/2016 10:42 AM, paul_kon...@dell.com wrote:



Would you expect people to conform to the abridged version or the
full standard?  If the full standard, then publishing an abridged
version is not a good idea, it will just cause confusion.  Let the
full standard be the rule, make people read it, and if they didn't
bother that's their problem.


I agree; let's not have two documents that can conflict or get out of
sync with each other, unless you can figure out how to extract the
abridged document automatically from the full version.


Hmm. As for being out-of-date, I'd say the official document is guilty: 
it talks about how we can use C89 now that it is prevalent enough. Now 
that I've looked at it again after many years, the document really does 
seem poorly organized and contains lots of irrelevant information (a 
huge table of long option names?)


I think if we limit our local document to just the basics (and maybe 
call it a Getting Started guide rather than an abridged form of the GNU 
coding standards), there's little danger of it going out of date, and I 
still think having it would improve our documentation.



Bernd

Re: [PATCH, www] Fix typo in htdocs/develop.html

[Bernd Schmidt]

On 04/21/2016 02:16 PM, Kirill Yukhin wrote:

Hello,
This looks like a typo to me.

   GCC 6 Stage 4 (starts 2016-01-20)GCC 5.3 release (2015-12-04)
|
+-- GCC 5 branch created +
| \
v  v

Patch in the bottom. Is it ok to install?


Sure.


Bernd

Re: Allow embedded timestamps by C/C++ macros to be set externally (3)

[Bernd Schmidt]

On 04/18/2016 02:26 PM, Dhole wrote:

A few months ago I submited a patch to allow the embedded timestamps by
C/C++ macros to be set externally [2], which was already an improvement
over [1].  I was told to wait until the GCC 7 stage 1 started to send
this patch again.



+/* Read SOURCE_DATE_EPOCH from environment to have a deterministic
+   timestamp to replace embedded current dates to get reproducible
+   results. Returns -1 if SOURCE_DATE_EPOCH is not defined.  */
+long long
+get_source_date_epoch()


Always have a space before open-paren. Maybe this should return time_t. 
See below.



+/* Read SOURCE_DATE_EPOCH from environment to have a deterministic
+   timestamp to replace embedded current dates to get reproducible
+   results. Returns -1 if SOURCE_DATE_EPOCH is not defined.  */
+extern long long get_source_date_epoch();


Double space after the end of a sentence. Space before open paren.


+  source_date_epoch = get_source_date_epoch();
+  cpp_init_source_date_epoch(parse_in, source_date_epoch);


Spaces.


+/* Initialize the source_date_epoch value.  */
+extern void cpp_init_source_date_epoch (cpp_reader *, long long);


Also thinking we should be using time_t here.


  /* Sanity-checks are dependent on command-line options, so it is
 called as a subroutine of cpp_read_main_file ().  */


We don't write () to mark function names.


+tb = gmtime ((time_t*) >source_date_epoch);


Space before the "*". But this cast looks ugly and unreliable (think 
big-endian). This is why I would prefer to move to a time_t 
representation sooner.



2016-04-18  Eduard Sanou
Matthias Klose
* c-common.c (get_source_date_epoch): New function, gets the environment
variable SOURCE_DATE_EPOCH and parses it as long long with error
handling.
* c-common.h (get_source_date_epoch): Prototype.
* c-lex.c (c_lex_with_flags): set parse_in->source_date_epoch.


Add blank lines after the end of the names in ChangeLogs.


Bernd

Re: [DOC Patch] Add sample for @cc constraint

[Bernd Schmidt]

On 04/16/2016 01:12 AM, David Wohlferd wrote:

There were  basically 3 changes I was trying for in that doc patch. Are
any of them worth keeping?  Or are we done?

1) "Do not clobber flags if they are being used as outputs."
2) Output flags sample (with #if removed).
3) "On the x86 platform, flags are always treated as clobbered by
extended asm whether @code{"cc"} is specified or not."

I'm prepared to send an updated patch if there's anything here that
might get approved.


I think the updated flags sample would be nice to have.


Bernd

Re: [PATCH] Don't build 32-bit libatomic with -march=i486 on x86-64

[Bernd Schmidt]

On 04/20/2016 04:57 PM, H.J. Lu wrote:

On Wed, Apr 20, 2016 at 7:54 AM, Jakub Jelinek  wrote:

https://gcc.gnu.org/ml/gcc-patches/2016-04/msg01080.html


This is wrong, see my other comment on the libgomp patch.


See my reply to your reply on the libgomp patch.


Since Jakub has said it is wrong, please revert.


Bernd

Re: [PATCH 01/18] stop using rtx_insn_list in reorg.c

[Bernd Schmidt]

On 04/20/2016 08:22 AM, tbsaunde+...@tbsaunde.org wrote:

-  rtx_insn_list *merged_insns = 0;
+  auto_vec merged_insns;


I see Jeff has already acked this, but some of the expressions here are 
getting unwieldy. can we maybe shorten some of this using typedefs?



Bernd

Re: IRA costs tweaks, PR 56069

[Bernd Schmidt]

On 03/02/2016 10:53 PM, Vladimir Makarov wrote:



The patch is ok for me.  But I'd wait for GCC7.  People are sensitive to
their code performance degradation.  Even in most cases the patch
improves performance, in some case it can worsen code and people might
fill new PRs after such change.


I've retested now, and it turns out that it turns two guality/sra-1.c 
tests into UNSUPPORTED. Apparently a variable is now destructively 
overwritten. Is that something we worry about (I'm inclined to think no)?



Bernd

Re: C, C++: New warning for memset without multiply by elt size

[Bernd Schmidt]

On 04/22/2016 03:57 PM, Jason Merrill wrote:

This looks good, but can we move the code into c-common rather than
duplicate it?


That would be this patch. Also passes testing on x86_64-linux.


Bernd
	* doc/invoke.texi (Warning Options): Add -Wmemset-elt-size.
	(-Wmemset-elt-size): New item.
c-family/
	* c.opt (Wmemset-elt-size): New option.
	* c-common.c (warn_for_memset): New function.
	* c-common.h (warn_for_memset): Declare.
c/
	* c-parser.c (c_parser_postfix_expression_after_primary): Call
	warn_for_memset instead of warning directly here.
cp/
	* parser.c (cp_parser_postfix_expression): Call
	warn_for_memset instead of warning directly here.
testsuite/
	* c-c++-common/memset-array.c: New test.

Index: gcc/c/c-parser.c
===
--- gcc/c/c-parser.c	(revision 235384)
+++ gcc/c/c-parser.c	(working copy)
@@ -8282,18 +8282,15 @@ c_parser_postfix_expression_after_primar
 	  expr.value, exprlist,
 	  sizeof_arg,
 	  sizeof_ptr_memacc_comptypes);
-	  if (warn_memset_transposed_args
-	  && TREE_CODE (expr.value) == FUNCTION_DECL
+	  if (TREE_CODE (expr.value) == FUNCTION_DECL
 	  && DECL_BUILT_IN_CLASS (expr.value) == BUILT_IN_NORMAL
 	  && DECL_FUNCTION_CODE (expr.value) == BUILT_IN_MEMSET
-	  && vec_safe_length (exprlist) == 3
-	  && integer_zerop ((*exprlist)[2])
-	  && (literal_zero_mask & (1 << 2)) != 0
-	  && (!integer_zerop ((*exprlist)[1])
-		  || (literal_zero_mask & (1 << 1)) == 0))
-	warning_at (expr_loc, OPT_Wmemset_transposed_args,
-			"% used with constant zero length parameter; "
-			"this could be due to transposed parameters");
+	  && vec_safe_length (exprlist) == 3)
+	{
+	  tree arg0 = (*exprlist)[0];
+	  tree arg2 = (*exprlist)[2];
+	  warn_for_memset (expr_loc, arg0, arg2, literal_zero_mask);
+	}
 
 	  start = expr.get_start ();
 	  finish = parser->tokens_buf[0].get_finish ();
Index: gcc/c-family/c-common.c
===
--- gcc/c-family/c-common.c	(revision 235384)
+++ gcc/c-family/c-common.c	(working copy)
@@ -11767,6 +11767,49 @@ warn_for_div_by_zero (location_t loc, tr
 warning_at (loc, OPT_Wdiv_by_zero, "division by zero");
 }
 
+/* Warn for patterns where memset appears to be used incorrectly.  The
+   warning location should be LOC.  ARG0, and ARG2 are the first and
+   last arguments to the call, while LITERAL_ZERO_MASK has a 1 bit for
+   each argument that was a literal zero.  */
+
+void
+warn_for_memset (location_t loc, tree arg0, tree arg2,
+		 int literal_zero_mask)
+{
+  if (warn_memset_transposed_args
+  && integer_zerop (arg2)
+  && (literal_zero_mask & (1 << 2)) != 0
+  && (literal_zero_mask & (1 << 1)) == 0)
+warning_at (loc, OPT_Wmemset_transposed_args,
+		"% used with constant zero length "
+		"parameter; this could be due to transposed "
+		"parameters");
+
+  if (warn_memset_elt_size && TREE_CODE (arg2) == INTEGER_CST)
+{
+  STRIP_NOPS (arg0);
+  if (TREE_CODE (arg0) == ADDR_EXPR)
+	arg0 = TREE_OPERAND (arg0, 0);
+  tree type = TREE_TYPE (arg0);
+  if (TREE_CODE (type) == ARRAY_TYPE)
+	{
+	  tree elt_type = TREE_TYPE (type);
+	  tree domain = TYPE_DOMAIN (type);
+	  if (!integer_onep (TYPE_SIZE_UNIT (elt_type))
+	  && TYPE_MAXVAL (domain)
+	  && TYPE_MINVAL (domain)
+	  && integer_zerop (TYPE_MINVAL (domain))
+	  && integer_onep (fold_build2 (MINUS_EXPR, domain,
+	arg2,
+	TYPE_MAXVAL (domain
+	warning_at (loc, OPT_Wmemset_elt_size,
+			"% used with length equal to "
+			"number of elements without multiplication "
+			"with element size");
+	}
+}  
+}
+
 /* Subroutine of build_binary_op. Give warnings for comparisons
between signed and unsigned quantities that may fail. Do the
checking based on the original operand trees ORIG_OP0 and ORIG_OP1,
Index: gcc/c-family/c-common.h
===
--- gcc/c-family/c-common.h	(revision 235384)
+++ gcc/c-family/c-common.h	(working copy)
@@ -902,6 +902,7 @@ extern void c_parse_file (void);
 extern void c_parse_final_cleanups (void);
 
 extern void warn_for_omitted_condop (location_t, tree);
+extern void warn_for_memset (location_t, tree, tree, int);
 
 /* These macros provide convenient access to the various _STMT nodes.  */
 
Index: gcc/c-family/c.opt
===
--- gcc/c-family/c.opt	(revision 235384)
+++ gcc/c-family/c.opt	(working copy)
@@ -565,6 +565,10 @@ Wmemset-transposed-args
 C ObjC C++ ObjC++ Var(warn_memset_transposed_args) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
 Warn about suspicious calls to memset where the third argument is constant literal zero and the second is not.
 
+Wmemset-elt-size
+C ObjC C++ ObjC++ Var(warn_memset_elt_size) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
+Warn about 

Re: [PATCH] Fix missed DSE opportunity with operator delete.

[Bernd Schmidt]

On 04/19/2016 10:48 PM, Mikhail Maltsev wrote:

On 04/18/2016 12:14 PM, Richard Biener wrote:


Enlarging tree_function_decl is bad.

Probably using 3 bits for malloc_flag, operator_new_flag and free_flag is
redundant. I packed the state into 2 bits.


Passes should get at the info via flags_from_decl_or_type () and a new
ECF_FREE.

Fixed.


I think we should also have a testcase that verifies that no DSE happens 
for something that has a destructor.



Bernd

An abridged "Writing C" for the gcc web pages

[Bernd Schmidt]

(Apologies if you get this twice, the mailing list didn't like the html 
attachment in the first attempt).


We frequently get malformatted patches, and it's been brought to my 
attention that some people don't even make the effort to read the GNU 
coding standards before trying to contribute code. TL;DR seems to be the 
excuse, and while I find that attitude inappropriate, we could probably 
improve the situation by spelling out the most basic rules in an 
abridged document on our webpages. Below is a draft I came up with. 
Thoughts?



Bernd

Index: htdocs/contribute.html
===
RCS file: /cvs/gcc/wwwdocs/htdocs/contribute.html,v
retrieving revision 1.87
diff -d -u -r1.87 contribute.html
--- htdocs/contribute.html	9 Apr 2015 21:49:31 -	1.87
+++ htdocs/contribute.html	22 Apr 2016 16:19:12 -
@@ -61,7 +61,9 @@
 
 All contributions must conform to the http://www.gnu.org/prep/standards_toc.html;>GNU Coding
-Standards.  There are also some .  An abridged version of
+the C formatting guidelines is available for purposes of getting
+started.  There are also some additional coding conventions for
 GCC; these include documentation and testsuite requirements as
 well as requirements on code formatting.
--- /dev/null	2016-04-22 12:54:13.474856906 +0200
+++ htdocs/writing-c.html	2016-04-22 18:11:47.222034004 +0200
@@ -0,0 +1,263 @@
+
+
+
+
+Getting Started Writing C for the GCC project
+
+
+
+
+Introduction
+
+This guide is intended as a heavily abridged version of the C
+coding guidelines in the full http://www.gnu.org/prep/standards_toc.html;>GNU Coding
+Standards, which may be too voluminous for someone just looking to
+contribute a short patch to GCC.  Here, the focus is mainly on the
+very basic formatting rules and how they apply to GCC.  Anyone who
+wishes to become a long-time contributor should still read the full
+document.  There is also https://gcc.gnu.org/codingconventions.html;>an additional
+document going into considerably more detail about less common
+cases that may arise.
+
+We care very strongly about ensuring all code in GCC is formatted
+the same way, as inconsistencies can be very distracting and make it
+harder to modify the source.  As always, there are exceptions to the
+rules, but they should be rare and only made for good reasons.
+
+When we speak about C formatting guidelines in this document, we
+intend them to be applied to C++ code as well.  Obviously there are
+certain situations where the use of C++ features creates new problems,
+and we have not settled on complete solutions in all cases.  The most
+important rule is this: when in doubt about anything, carefully
+examine existing code in the area you are changing (or analogous code
+in other files) and try to follow the prevalent style.
+
+Tools
+
+We strongly recommend using an editor which understands and
+supports the GNU formatting rules to some degree.  GNU emacs is
+designed for this purpose and supports GNU style as the default for C
+formatting, and we recommend it for this reason.  However, there are
+also contributors making good use of other editors such as vim.  Using
+more exotic tools, such as Eclipse, or Windows-based editors, may
+prove problematic.
+
+GCC has a check_GNU_style.sh script in the
+contrib/ directory to verify the formatting in a patch
+file.  Some caution is advisable however, as machines are generally
+not very good at applying common sense.
+
+To make sure that a correctly formatted patch doesn't get destroyed
+by email software, it is usually best to send it as an attachment.
+The format should be text/plain so that mail clients such as
+thunderbird can display and quote it, without forcing potential
+reviewers to take extra steps to save it and open it elsewhere before
+being able to look at it.
+
+Formatting Your Source Code
+
+Formatting Basics
+
+Please keep the length of source lines to 79 characters or less, for
+maximum readability in the widest range of environments.
+
+Tab characters are interpreted as jumps to stops separated by eight
+spaces.  Note that editors on certain systems (such as Microsoft
+Windows) may follow different rules by default, so pay special
+attention if you are using such systems.
+
+All leading whitespace should be replaced with tab characters as
+much as possible, but tab characters should not be used in any other
+circumstances. There should never be trailing whitespace, and no
+carriage-return characters at the line end.  Blank lines should just
+consist of a newline character.
+
+There should be a space before open-parentheses and after commas.
+We also use spaces around binary operators.
+
+Avoid unnecessary parentheses and braces, except in special cases that
+are described elsewhere in this document.  The following example
+shows how not to do it:
+  if ((a == 0) && (b == 2))
+{
+  return (z);
+}
+
+
+Instead, this should be written as follows:
+  if (a == 0 && b == 2)
+

Re: C, C++: New warning for memset without multiply by elt size

[Bernd Schmidt]

On 04/22/2016 05:31 PM, Jason Merrill wrote:

On Fri, Apr 22, 2016 at 11:24 AM, Bernd Schmidt <bernds_...@t-online.de> wrote:

On 04/22/2016 03:57 PM, Jason Merrill wrote:

This looks good, but can we move the code into c-common rather than
duplicate it?


Probably not without a fair amount of surgery, since the cdw_ and ds_ codes
are private to each frontend.


I don't see any cdw_ or ds_ in this patch.


Ah sorry, that was the other patch. I'm somewhat jetlagged. I'll look 
into this; I do remember there being some differences in the C and C++ 
parts, but it may just be the bit where it looks past a CONST_DECL for C++.



Bernd

Re: C, C++: New warning for memset without multiply by elt size

[Bernd Schmidt]

On 04/22/2016 03:57 PM, Jason Merrill wrote:

This looks good, but can we move the code into c-common rather than
duplicate it?


Probably not without a fair amount of surgery, since the cdw_ and ds_ 
codes are private to each frontend.



Bernd

C, C++: New warning for memset without multiply by elt size

[Bernd Schmidt]

We had this problem in the C frontend until very recently:

  int array[some_count];
  memset (array, 0, some_count);

which forgets to multiply by sizeof int. The following patch implements 
a new warning option for this.


Bootstrapped and tested (a while ago, will retest) on x86_64-linux. Ok 
for trunk?



Bernd
	* doc/invoke.texi (Warning Options): Add -Wmemset-elt-size.
	(-Wmemset-elt-size): New item.
c-family/
	* c.opt (Wmemset-elt-size): New option.
c/
	* c-parser.c (c_parser_postfix_expression_after_primary): Warn for
	memset with element count rather than array size.
cp/
	* parser.c (cp_parser_postfix_expression): Warn for
	memset with element count rather than array size.
testsuite/
	* c-c++-common/memset-array.c: New test.

Index: gcc/c/c-parser.c
===
--- gcc/c/c-parser.c	(revision 234183)
+++ gcc/c/c-parser.c	(working copy)
@@ -8240,18 +8240,46 @@ c_parser_postfix_expression_after_primar
 	  expr.value, exprlist,
 	  sizeof_arg,
 	  sizeof_ptr_memacc_comptypes);
-	  if (warn_memset_transposed_args
-	  && TREE_CODE (expr.value) == FUNCTION_DECL
+	  if (TREE_CODE (expr.value) == FUNCTION_DECL
 	  && DECL_BUILT_IN_CLASS (expr.value) == BUILT_IN_NORMAL
 	  && DECL_FUNCTION_CODE (expr.value) == BUILT_IN_MEMSET
-	  && vec_safe_length (exprlist) == 3
-	  && integer_zerop ((*exprlist)[2])
-	  && (literal_zero_mask & (1 << 2)) != 0
-	  && (!integer_zerop ((*exprlist)[1])
-		  || (literal_zero_mask & (1 << 1)) == 0))
-	warning_at (expr_loc, OPT_Wmemset_transposed_args,
-			"% used with constant zero length parameter; "
-			"this could be due to transposed parameters");
+	  && vec_safe_length (exprlist) == 3)
+	{
+	  if (warn_memset_transposed_args
+		  && integer_zerop ((*exprlist)[2])
+		  && (literal_zero_mask & (1 << 2)) != 0
+		  && (!integer_zerop ((*exprlist)[1])
+		  || (literal_zero_mask & (1 << 1)) == 0))
+		warning_at (expr_loc, OPT_Wmemset_transposed_args,
+			"% used with constant zero length "
+			"parameter; this could be due to transposed "
+			"parameters");
+	  if (warn_memset_elt_size
+		  && TREE_CODE ((*exprlist)[2]) == INTEGER_CST)
+		{
+		  tree ptr = (*exprlist)[0];
+		  STRIP_NOPS (ptr);
+		  if (TREE_CODE (ptr) == ADDR_EXPR)
+		ptr = TREE_OPERAND (ptr, 0);
+		  tree type = TREE_TYPE (ptr);
+		  if (TREE_CODE (type) == ARRAY_TYPE)
+		{
+		  tree elt_type = TREE_TYPE (type);
+		  tree domain = TYPE_DOMAIN (type);
+		  if (!integer_onep (TYPE_SIZE_UNIT (elt_type))
+			  && TYPE_MAXVAL (domain)
+			  && TYPE_MINVAL (domain)
+			  && integer_zerop (TYPE_MINVAL (domain))
+			  && integer_onep (fold_build2 (MINUS_EXPR, domain,
+			(*exprlist)[2],
+			TYPE_MAXVAL (domain
+		  warning_at (expr_loc, OPT_Wmemset_elt_size,
+  "% used with length equal to "
+  "number of elements without multiplication "
+  "with element size");
+		}
+		}
+	}
 
 	  start = expr.get_start ();
 	  finish = parser->tokens_buf[0].get_finish ();
Index: gcc/cp/parser.c
===
--- gcc/cp/parser.c	(revision 234183)
+++ gcc/cp/parser.c	(working copy)
@@ -6829,20 +6829,48 @@ cp_parser_postfix_expression (cp_parser
 		  }
 	  }
 
-	if (warn_memset_transposed_args)
+	if (TREE_CODE (postfix_expression) == FUNCTION_DECL
+		&& DECL_BUILT_IN_CLASS (postfix_expression) == BUILT_IN_NORMAL
+		&& DECL_FUNCTION_CODE (postfix_expression) == BUILT_IN_MEMSET
+		&& vec_safe_length (args) == 3)
 	  {
-		if (TREE_CODE (postfix_expression) == FUNCTION_DECL
-		&& DECL_BUILT_IN_CLASS (postfix_expression) == BUILT_IN_NORMAL
-		&& DECL_FUNCTION_CODE (postfix_expression) == BUILT_IN_MEMSET
-		&& vec_safe_length (args) == 3
-		&& TREE_CODE ((*args)[2]) == INTEGER_CST
-		&& integer_zerop ((*args)[2])
-		&& !(TREE_CODE ((*args)[1]) == INTEGER_CST
-			 && integer_zerop ((*args)[1])))
+		tree arg1 = (*args)[1];
+		tree arg2 = (*args)[2];
+		if (TREE_CODE (arg2) == CONST_DECL)
+		  arg2 = DECL_INITIAL (arg2);
+
+		if (warn_memset_transposed_args
+		&& integer_zerop (arg2)
+		&& !(TREE_CODE (arg1) == INTEGER_CST
+			 && integer_zerop (arg1)))
 		  warning (OPT_Wmemset_transposed_args,
 			   "% used with constant zero length "
 			   "parameter; this could be due to transposed "
 			   "parameters");
+		if (warn_memset_elt_size && TREE_CODE (arg2) == INTEGER_CST)
+		  {
+		tree ptr = (*args)[0];
+		STRIP_NOPS (ptr);
+		if (TREE_CODE (ptr) == ADDR_EXPR)
+		  ptr = TREE_OPERAND (ptr, 0);
+		tree type = TREE_TYPE (ptr);
+		if (TREE_CODE (type) == ARRAY_TYPE)
+		  {
+			tree elt_type = TREE_TYPE (type);
+			tree domain = TYPE_DOMAIN (type);
+			if (!integer_onep (TYPE_SIZE_UNIT (elt_type))
+			&& TYPE_MAXVAL (domain)
+			&& TYPE_MINVAL (domain)
+			&& integer_zerop 

C, C++: Fix PR 69733 (bad location for ignored qualifiers warning)

[Bernd Schmidt]

The PR is for a C++ form of the form

const double val() const { ... }

where the warning location is at the second const (by accident, in 
reality it's just past the function's declarator), while the first const 
is the one that we are warning about.


This patch adds some logic to the C and C++ frontends to look for the 
qualifier, or a typedef name, and point the warning there. C needs a 
little more work because the ignored qualifier could also be an address 
space.


Bootstrapped and tested on x86_64-linux (a while ago, will retest). Ok 
for trunk?



Bernd
c/
	PR c++/69733
	* c-decl.c (smallest_type_quals_location): New static function.
	(grokdeclarator): Try to find the correct location for an ignored
	qualifier.
cp/
	PR c++/69733
	* decl.c (grokdeclarator): Try to find the correct location for an
	ignored qualifier.
testsuite/
	PR c++/69733
	* c-c++-common/pr69733.c: New test.
	* gcc.target/i386/pr69733.c: New test.

Index: gcc/c/c-decl.c
===
--- gcc/c/c-decl.c	(revision 234183)
+++ gcc/c/c-decl.c	(working copy)
@@ -5291,6 +5291,27 @@ warn_defaults_to (location_t location, i
   va_end (ap);
 }
 
+/* Returns the smallest location != UNKNOWN_LOCATION in LOCATIONS,
+   considering only those c_declspec_words found in LIST, which
+   must be terminated by cdw_number_of_elements.  */
+
+static location_t
+smallest_type_quals_location (const location_t* locations,
+			  c_declspec_word *list)
+{
+  location_t loc = UNKNOWN_LOCATION;
+  while (*list != cdw_number_of_elements)
+{
+  location_t newloc = locations[*list];
+  if (loc == UNKNOWN_LOCATION
+	  || (newloc != UNKNOWN_LOCATION && newloc < loc))
+	loc = newloc;
+  list++;
+}
+
+  return loc;
+}
+
 /* Given declspecs and a declarator,
determine the name and type of the object declared
and construct a ..._DECL node for it.
@@ -6101,6 +6122,18 @@ grokdeclarator (const struct c_declarato
 	   qualify the return type, not the function type.  */
 	if (type_quals)
 	  {
+		enum c_declspec_word ignored_quals_list[] =
+		  {
+		cdw_const, cdw_volatile, cdw_restrict, cdw_address_space,
+		cdw_number_of_elements
+		  };
+		location_t specs_loc
+		= smallest_type_quals_location (declspecs->locations,
+		ignored_quals_list);
+		if (specs_loc == UNKNOWN_LOCATION)
+		  specs_loc = declspecs->locations[cdw_typedef];
+		if (specs_loc == UNKNOWN_LOCATION)
+		  specs_loc = loc;
 		/* Type qualifiers on a function return type are
 		   normally permitted by the standard but have no
 		   effect, so give a warning at -Wreturn-type.
@@ -6108,10 +6141,10 @@ grokdeclarator (const struct c_declarato
 		   function definitions in ISO C; GCC used to used
 		   them for noreturn functions.  */
 		if (VOID_TYPE_P (type) && really_funcdef)
-		  pedwarn (loc, 0,
+		  pedwarn (specs_loc, 0,
 			   "function definition has qualified void return type");
 		else
-		  warning_at (loc, OPT_Wignored_qualifiers,
+		  warning_at (specs_loc, OPT_Wignored_qualifiers,
 			   "type qualifiers ignored on function return type");
 
 		type = c_build_qualified_type (type, type_quals);
Index: gcc/cp/decl.c
===
--- gcc/cp/decl.c	(revision 234183)
+++ gcc/cp/decl.c	(working copy)
@@ -10010,8 +10010,15 @@ grokdeclarator (const cp_declarator *dec
 	if (type_quals != TYPE_UNQUALIFIED)
 	  {
 		if (SCALAR_TYPE_P (type) || VOID_TYPE_P (type))
-		  warning (OPT_Wignored_qualifiers,
-			   "type qualifiers ignored on function return type");
+		  {
+		location_t loc;
+		loc = smallest_type_quals_location (type_quals,
+			declspecs->locations);
+		if (loc == UNKNOWN_LOCATION)
+		  loc = declspecs->locations[ds_type_spec];
+		warning_at (loc, OPT_Wignored_qualifiers, "type "
+"qualifiers ignored on function return type");
+		  }
 		/* We now know that the TYPE_QUALS don't apply to the
 		   decl, but to its return type.  */
 		type_quals = TYPE_UNQUALIFIED;
Index: gcc/testsuite/c-c++-common/pr69733.c
===
--- gcc/testsuite/c-c++-common/pr69733.c	(revision 0)
+++ gcc/testsuite/c-c++-common/pr69733.c	(working copy)
@@ -0,0 +1,24 @@
+/* { dg-do compile } */
+/* { dg-options "-W -fdiagnostics-show-caret" } */
+
+typedef const double cd;
+double val;
+
+const double val0() {return val;} /* { dg-warning "qualifiers ignored" } */
+/* { dg-begin-multiline-output "" }
+ const double val0() {return val;}
+ ^
+{ dg-end-multiline-output "" } */
+
+volatile double val1() {return val;} /* { dg-warning "qualifiers ignored" } */
+/* { dg-begin-multiline-output "" }
+ volatile double val1() {return val;}
+ ^~~~
+{ dg-end-multiline-output "" } */
+
+cd val2() {return val;} /* { dg-warning "qualifiers ignored" } */
+/* { dg-begin-multiline-output "" }
+ cd val2() {return val;}
+ ^~
+{ dg-end-multiline-output "" } */
+
Index: 

Re: Fix some x86 peepholes vs. the red-zone

[Bernd Schmidt]

On 04/15/2016 10:32 AM, Uros Bizjak wrote:

This fixes possible wrong code with a tricky failure mode, so OK now.


Thanks.

[...]


While changing this part, it can be rewritten using dg-additional options, e.g.:

/* { dg-options "-Os -fomit-frame-pointer -fasynchronous-unwind-tables
-mno-red-zone" } */
/* { dg-additional-options "-mpreferred-stack-boundary=3" { target ia32 } } */


Since Jakub mentioned branching today, and I wasn't sure I'd have time 
for a retest, I've committed it as-is.



Bernd

Re: Fix for PR70498 in Libiberty Demangler

[Bernd Schmidt]

On 04/13/2016 03:04 PM, Marcel Böhme wrote:

Hi Bernd,

Shouldn't we check for overflows before performing the +1 addition
(i.e. 0 <= num < INT_MAX)? Ideally we'd also have a way to signal
from d_number if we had an overflow while parsing that number.

Without an overflow signal, d_number will already be prone to return
a negative number for supposedly non-negative numbers (those not
preceded with ’n’). In that case an overflow check would be
unnecessary in d_compact_number which is supposed to always return a
positive number or a negative one (-1). If you decide in favour of an
overflow signal, it must be handled by the call-sites. Not sure what
the “default behaviour” should be then. Otherwise, we can simply
assume that the call sites for d_number can handle negative numbers.


Shouldn't we look into fixing d_number eventually so it can signal error?


index = d_compact_number (di) + 1; if (index == 0) return NULL;

which probably ought to have the same kind of check (I'll note that
at this point we've accumulated two "+1"s, I'll assume that's what
we want).

Yes. There should be an overflow check here.


Could you update the patch for that?


Bernd

Re: [PATCH] rtlanal: Fix bits/bytes confusion in set_noop_p (PR68814)

[Bernd Schmidt]

On 04/15/2016 02:35 AM, Segher Boessenkool wrote:

This now also shows up on GCC 5, see PR70672.  It there happens in
the jump1 pass already.

Is this okay to backport to 5 and 4.9?


Ok.


Bernd

Re: Splitting up gcc/omp-low.c?

[Bernd Schmidt]

On 04/14/2016 03:36 PM, Thomas Schwinge wrote:


I don't know how meaningful it is to create/discuss/review/commit the
following patch until the overall approach has been agreed upon?


Why not? We agree the file should be split, and this makes it easier. So 
I'll approve it for stage1, or earlier if RMs want this split in gcc-6.


You could however probably also remove the #include langhooks from the C 
file.



Bernd

Re: Splitting up gcc/omp-low.c?

[Bernd Schmidt]

On 04/13/2016 07:56 PM, Thomas Schwinge wrote:


Best way to present this might be to do
diff -du old-omp-low.c .


OK, I found Git "-C5%" produce something very similar to that;
0001-Split-up-gcc-omp-low.c-plain.patch.xz attached.


That looks much better. However, the //OMPWHATEVER comments are not 
really all that helpful. I think the next step would be to clear out all 
this stuff including the OMPCUT markers, and also to start with an 
initial patch that includes everything that actually modifies code 
rather than moving it (omp_is_reference seems to be the major thing here).



Bernd

Re: Splitting up gcc/omp-low.c?

[Bernd Schmidt]

On 04/13/2016 06:01 PM, Thomas Schwinge wrote:


The attached 0001-Split-up-gcc-omp-low.c.patch.xz is a Git "--color
--word-diff --ignore-space-change" patch, purely meant for manual review;
I'm intentionally ;-) not attaching a "patch-applyable" patch at this
point, to minimize the risk of other people starting to work on this in
parallel with my ongoing changes, which no doubt would result in a ton of
patch merge conflicts.  Yet, I'm of course very open to any kind of
suggestions; please submit these as a "verbal patch".  I will of course
submit a patch in any other format that you'd like for review.


I have no idea how to read this patch. I can't even properly show it 
with "less" because it seems to contain color escape sequences. The 
word-diff format (I assume that's what it is) is also unfamiliar and not 
immediately readable.


Best way to present this might be to do
diff -du old-omp-low.c .


Bernd

Re: PATCH for c++/70639 (ICE-on-valid with -Wmisleading-indentation and switch)

[Bernd Schmidt]

On 04/13/2016 06:11 PM, David Malcolm wrote:

Your approach encapsulates the logic for rejecting this situation
within should_warn_for_misleading_indentation, rather than at the
callers, which is arguably better for modularity (similar to how we
already call it for "do", which is then always rejected).

So your patch looks good to me (I don't have formal approval rights for
this though).


I'll ack it, I also thought it looked reasonable.


Bernd

Re: C/C++ PATCH to add -Wdangling-else option

[Bernd Schmidt]

On 04/13/2016 04:14 PM, Marek Polacek wrote:

This patch is meant to be applied on top of the "Wparentheses overhaul" patch.

I really think that warning about the dangling else problem isn't appropriate
as a part of the -Wparentheses warning, which I think should only deal with
stuff like precedence of operators, i.e. things where ()'s are missing and not
{}'s.

This new warning is, however, a subset of -Wparentheses.

Bootstrapped/regtested on x86_64-linux, ok for trunk or should I stash it
for the next stage1?


I think it's not appropriate for now. I'm ambivalent about the concept; 
my (vague) recollection is that putting it under -Wparentheses was 
Kenner's idea, and it's been there so long that I'm not sure there's 
really a point to changing this. In a sense it is a very similar problem 
as operator precedence.



Bernd

Re: C PATCH to overhaul warning about dangling else (PR c/70436)

[Bernd Schmidt]

On 04/13/2016 04:14 PM, Marek Polacek wrote:

I revamped the warning so that it follows what the C++ FE does (i.e.  passing
IF_P bools here and there) and it seems to work quite well.  I didn't mean to
tackle the OMP bits but I bet it would be just a matter of passing IF_P
somewhere.

Bootstrapped/regtested on x86_64-linux, ok for trunk?


Ok if you adjust function comments to mention the new argument where 
applicable.



Bernd

Re: [PATCH] Fix debug ICE on aarch64 due to bad rtl simplification (PR debug/70628)

[Bernd Schmidt]

On 04/12/2016 10:34 PM, Jakub Jelinek wrote:

+ return temp ? gen_rtx_fmt_ee (GET_CODE (x), to_mode,
+   temp, XEXP (x, 1))
+ : temp;


Wrap multi-line expressions in parens. No need for a full retest, just 
make sure the file still builds.



Bernd

Re: [PATCH] Fix debug ICE on aarch64 due to bad rtl simplification (PR debug/70628)

[Bernd Schmidt]

On 04/12/2016 10:06 PM, Jakub Jelinek wrote:

On Tue, Apr 12, 2016 at 10:02:18PM +0200, Bernd Schmidt wrote:

On 04/12/2016 05:10 PM, Jakub Jelinek wrote:

This patch arranges for a new argument to
convert_memory_address_addr_space_1 and calls it with that new argument set
to true, to make sure it never emits instructions or creates pseudos.


I think the approach looks sensible, but I don't know if you need the extra
argument. It looks like convert_memory_address_addr_space_1 currently only
has one user, can't you move the convert_modes call up into that function so
that convert_memory_address_addr_space_1 never emits anything?


convert_memory_address_addr_space_1 is recursive function, so it has more
than one caller (convert_memory_address_addr_space and itself).


D'oh. Sorry.


Which actually means that the patch is wrong, for no_emit on the recursive
calls it should actually check the result of the recursive call and
return NULL immediately if the recursive call returned NULL.


Hmm. I wonder whether this couldn't be restructured a bit to strip CONST 
internally and putting it back at the end. However, now is not the time 
for that.



So, would you be ok with the patch if I change it that way (to be tested
tomorrow)?


Yeah, I think so.


Bernd

Re: [PATCH] Fix debug ICE on aarch64 due to bad rtl simplification (PR debug/70628)

[Bernd Schmidt]

On 04/12/2016 05:10 PM, Jakub Jelinek wrote:

This patch arranges for a new argument to
convert_memory_address_addr_space_1 and calls it with that new argument set
to true, to make sure it never emits instructions or creates pseudos.


I think the approach looks sensible, but I don't know if you need the 
extra argument. It looks like convert_memory_address_addr_space_1 
currently only has one user, can't you move the convert_modes call up 
into that function so that convert_memory_address_addr_space_1 never 
emits anything?



Bernd

Re: [DOC Patch] Add sample for @cc constraint

[Bernd Schmidt]

On 04/12/2016 12:49 AM, David Wohlferd wrote:

First draft is attached.  It tests all 28 (14 conditions plus 14
inverted).  I wasn't sure what to set for optimization (O2? O3? O0?),
so I left the default.


I've had it in a successful test run, and committed it with a minor 
tweak (__builtin_abort vs return 1).


As for the docs, I think you are unnecessarily worried about things that 
are never going to be a problem in practice.



Bernd

Fix some x86 peepholes vs. the red-zone

[Bernd Schmidt]

With some changes I was working on, I produced a situation where one of 
the x86 peepholes made an incorrect transformation. In the prologue 
expansion code, we have


/* When using red zone we may start register saving before allocating
   the stack frame saving one cycle of the prologue.  However, avoid
   doing this if we have to probe the stack; at least on x86_64 the
   stack probe can turn into a call that clobbers a red zone location.

So, we can in some situations produce something like:

mov %ebx, -8(%rsp)
mov %edi, -16(%rsp)
subl $16, %rsp

which is fine if using a redzone. The problem is that there are 
peepholes which convert sp subtractions into pushes, clobbering the 
saved registers.


I've made these peepholes conditional on !x86_using_red_zone. 
Bootstrapped and tested on x86_64-linux, ok (now or later)?



Bernd
	* config/i386/i386-protos.h (ix86_using_red_zone): Declare.
	* config/i386/i386.c (ix86_using_red_zone): No longer static.
	* config/i386/i386.md (stack decrement to push peepholes): Guard
	with !x86_using_red_zone ().

testsuite/
	* gcc.target/i386/pr46470.c: Add -mno-red-zone to dg-optios for
	x86_64.

Index: gcc/config/i386/i386-protos.h
===
--- gcc/config/i386/i386-protos.h	(revision 234831)
+++ gcc/config/i386/i386-protos.h	(working copy)
@@ -44,6 +44,8 @@ extern bool ix86_use_pseudo_pic_reg (voi
 
 extern void ix86_reset_previous_fndecl (void);
 
+extern bool ix86_using_red_zone (void);
+
 #ifdef RTX_CODE
 extern int standard_80387_constant_p (rtx);
 extern const char *standard_80387_constant_opcode (rtx);
Index: gcc/config/i386/i386.c
===
--- gcc/config/i386/i386.c	(revision 234831)
+++ gcc/config/i386/i386.c	(working copy)
@@ -3709,7 +3709,7 @@ make_pass_stv (gcc::context *ctxt)
 
 /* Return true if a red-zone is in use.  */
 
-static inline bool
+bool
 ix86_using_red_zone (void)
 {
   return TARGET_RED_ZONE && !TARGET_64BIT_MS_ABI;
Index: gcc/config/i386/i386.md
===
--- gcc/config/i386/i386.md	(revision 234831)
+++ gcc/config/i386/i386.md	(working copy)
@@ -18240,7 +18240,8 @@ (define_peephole2
 	  (clobber (reg:CC FLAGS_REG))
 	  (clobber (mem:BLK (scratch)))])]
   "(TARGET_SINGLE_PUSH || optimize_insn_for_size_p ())
-   && INTVAL (operands[0]) == -GET_MODE_SIZE (word_mode)"
+   && INTVAL (operands[0]) == -GET_MODE_SIZE (word_mode)
+   && !ix86_using_red_zone ()"
   [(clobber (match_dup 1))
(parallel [(set (mem:W (pre_dec:P (reg:P SP_REG))) (match_dup 1))
 	  (clobber (mem:BLK (scratch)))])])
@@ -18253,7 +18254,8 @@ (define_peephole2
 	  (clobber (reg:CC FLAGS_REG))
 	  (clobber (mem:BLK (scratch)))])]
   "(TARGET_DOUBLE_PUSH || optimize_insn_for_size_p ())
-   && INTVAL (operands[0]) == -2*GET_MODE_SIZE (word_mode)"
+   && INTVAL (operands[0]) == -2*GET_MODE_SIZE (word_mode)
+   && !ix86_using_red_zone ()"
   [(clobber (match_dup 1))
(set (mem:W (pre_dec:P (reg:P SP_REG))) (match_dup 1))
(parallel [(set (mem:W (pre_dec:P (reg:P SP_REG))) (match_dup 1))
@@ -18267,7 +18269,8 @@ (define_peephole2
 			   (match_operand:P 0 "const_int_operand")))
 	  (clobber (reg:CC FLAGS_REG))])]
   "(TARGET_SINGLE_PUSH || optimize_insn_for_size_p ())
-   && INTVAL (operands[0]) == -GET_MODE_SIZE (word_mode)"
+   && INTVAL (operands[0]) == -GET_MODE_SIZE (word_mode)
+   && !ix86_using_red_zone ()"
   [(clobber (match_dup 1))
(set (mem:W (pre_dec:P (reg:P SP_REG))) (match_dup 1))])
 
@@ -18278,7 +18281,8 @@ (define_peephole2
 			   (match_operand:P 0 "const_int_operand")))
 	  (clobber (reg:CC FLAGS_REG))])]
   "(TARGET_DOUBLE_PUSH || optimize_insn_for_size_p ())
-   && INTVAL (operands[0]) == -2*GET_MODE_SIZE (word_mode)"
+   && INTVAL (operands[0]) == -2*GET_MODE_SIZE (word_mode)
+   && !ix86_using_red_zone ()"
   [(clobber (match_dup 1))
(set (mem:W (pre_dec:P (reg:P SP_REG))) (match_dup 1))
(set (mem:W (pre_dec:P (reg:P SP_REG))) (match_dup 1))])
Index: gcc/testsuite/gcc.target/i386/pr46470.c
===
--- gcc/testsuite/gcc.target/i386/pr46470.c	(revision 234831)
+++ gcc/testsuite/gcc.target/i386/pr46470.c	(working copy)
@@ -4,7 +4,7 @@
 /* These options are selected to ensure 1 word needs to be allocated
on the stack to maintain alignment for the call.  This should be
transformed to push+pop.  We also want to force unwind info updates.  */
-/* { dg-options "-Os -fomit-frame-pointer -fasynchronous-unwind-tables" } */
+/* { dg-options "-Os -fomit-frame-pointer -fasynchronous-unwind-tables -mno-red-zone" } */
 /* { dg-options "-Os -fomit-frame-pointer -mpreferred-stack-boundary=3 -fasynchronous-unwind-tables" { target ia32 } } */
 /* ms_abi has reserved stack-region.  */
 /* { dg-skip-if "" { x86_64-*-mingw* } { "*" } { "" } } */

Followup fix for PR69650

[Bernd Schmidt]

As shown by Roger Orr in the comments of this PR, cpp_get_token can 
cause a reallocation that invalidates some of the variables in 
do_linemarker. It seems to occur in this call to linemap_add:


  /* Allocate the new line_map.  However, if the current map only has a
 single line we can sometimes just increase its column_bits instead. */
  if (line_delta < 0
  || last_line != ORDINARY_MAP_STARTING_LINE_NUMBER (map)
  || SOURCE_COLUMN (map, highest) >= (1U << column_bits)
  || range_bits < map->m_range_bits)
map = linemap_check_ordinary
(const_cast 
  (linemap_add (set, LC_RENAME,
ORDINARY_MAP_IN_SYSTEM_HEADER_P (map),
ORDINARY_MAP_FILE_NAME (map),
to_line)));

I'm still not very familiar with this code, but I believe with LC_RENAME 
we'll retain the right information in INCLUDED_FROM even if map changes.


I've put his patch, slightly adapted, through a bootstrap and test cycle 
on x86_64-linux. Ok to apply?



Bernd
	PR preprocessor/69650
	* directives.c (do_linemarker): Reread map after calling
	cpp_get_token.

Index: libcpp/directives.c
===
--- libcpp/directives.c	(revision 234831)
+++ libcpp/directives.c	(working copy)
@@ -1048,6 +1048,9 @@ do_linemarker (cpp_reader *pfile)
 
   if (reason == LC_LEAVE)
 {
+  /* Reread map since cpp_get_token can invalidate it with a
+	 reallocation.  */
+  map = LINEMAPS_LAST_ORDINARY_MAP (line_table);
   const line_map_ordinary *from;  
   if (MAIN_FILE_P (map)
 	  || (new_file
@@ -1055,7 +1058,8 @@ do_linemarker (cpp_reader *pfile)
 	  && filename_cmp (ORDINARY_MAP_FILE_NAME (from), new_file) != 0))
 	{
 	  cpp_warning (pfile, CPP_W_NONE,
-		 "file \"%s\" linemarker ignored due to incorrect nesting", new_file);
+		   "file \"%s\" linemarker ignored due to "
+		   "incorrect nesting", new_file);
 	  return;
 	}
 }

Re: [PATCH] Don't add REG_EQUAL notes in fwprop for paradoxical subregs (PR rtl-optimization/70574)

[Bernd Schmidt]

On 04/08/2016 07:05 PM, Jakub Jelinek wrote:

After IRC discussions, I've bootstrapped/regtested following patch that
just punts if *loc contains any paradoxical subregs, together with
additional statistics gathering that proved that the new testcase is
the only spot in which this patch makes a difference on x86_64-linux
and i686-linux bootstrap/regtest.

Of course on other targets it might affect more.


I ran the test with just not generating any REG_EQUAL notes here in 
fwprop, and out of 4492 source files only three showed code generation 
differences, which suggests we're not getting much value out of these.


Two of the cases where the same missed tree optimization, for which I've 
filed PR70600.



2016-04-08  Jakub Jelinek  

PR rtl-optimization/70574
* fwprop.c (forward_propagate_and_simplify): Don't add
REG_EQUAL note if DF_REF_REG (use) is a paradoxical subreg.
(try_fwprop_subst): Don't add REG_EQUAL note if there are any
paradoxical subregs within *loc.

* gcc.target/i386/avx2-pr70574.c: New test.


Ok.


Bernd

Re: Fix for PR70498 in Libiberty Demangler

[Bernd Schmidt]

I've been looking through this patch. I had intended to commit it, but 
after looking through it a little more carefully I think there are a few 
things left to solve.


So, d_number/d_compact_number now return ints rather than longs, which 
makes sense since the lengths in things like struct demangle_component's 
s_name are integers. However, s_number there is defined as a long, so 
this does mean a tighter limit for things like 
d_template_param/d_make_template_param. Cc'ing Jason for an opinion on 
whether that's a problem or not (I suspect it isn't - t).



-static long
+static int
  d_compact_number (struct d_info *di)
  {
-  long num;
+  int num;
if (d_peek_char (di) == '_')
  num = 0;
else if (d_peek_char (di) == 'n')
@@ -2957,7 +2957,7 @@ d_compact_number (struct d_info *di)
else
  num = d_number (di) + 1;

-  if (! d_check_char (di, '_'))
+  if (num < 0 || ! d_check_char (di, '_'))
  return -1;
return num;
  }


Shouldn't we check for overflows before performing the +1 addition (i.e. 
0 <= num < INT_MAX)? Ideally we'd also have a way to signal from 
d_number if we had an overflow while parsing that number.


There's also this, in d_expression_1:

  index = d_compact_number (di) + 1;
  if (index == 0)
return NULL;

which probably ought to have the same kind of check (I'll note that at 
this point we've accumulated two "+1"s, I'll assume that's what we want).


Please include a ChangeLog entry with the next patch.


Bernd

Re: Fix for PR70492

[Bernd Schmidt]

On 04/01/2016 05:03 AM, Marcel Böhme wrote:

This fixes the invalid write of size 8 detailed in 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70492

Handle the special case when consume_count returns -1 due to an integer 
overflow when parsing the length of the virtual table qualifier in 
cplus-dem.c:2994 (gnu_special).

Index: libiberty/cplus-dem.c
===
--- libiberty/cplus-dem.c   (revision 234663)
+++ libiberty/cplus-dem.c   (working copy)
@@ -3001,6 +3001,11 @@ gnu_special (work, mangled, declp)
  success = 1;
  break;
}
+  else if (n == -1)
+{
+  success = 0;
+  break;
+}


I've discussed these patches with Jakub and he gave me RM green light 
for these patches at this stage. I've committed this one, and the one 
for PR69687.


Some more comments for next time. Patch submissions should include 
ChangeLog entries. For whitespace, try to follow the prevalent style 
which is to begin the line with tab characters; the patch above contains 
only spaces.



Bernd

Re: [PATCH] Don't add REG_EQUAL notes in fwprop for paradoxical subregs (PR rtl-optimization/70574)

[Bernd Schmidt]

On 04/07/2016 11:56 PM, Jakub Jelinek wrote:

Not sure if this patch catches everything though, perhaps there could be
e.g.
(set (reg:SI ...) (plus:SI ((subreg:SI (reg:QI ...) 0) (const_int ...)))
and we'd still assign REG_EQUAL note.  So maybe instead we should walk the
*loc expression and look for paradoxical subregs, and for each of them, if
we find the DF_REF_REG (use) mentioned in their operand, clear
set_reg_equal.  Though of course, if DF_REF_REG (use) itself is a
paradoxical subreg, we could clear set_reg_equal without any walking.


It seems like something like that could happen.

How much do we lose if we just don't make new REG_EQUAL notes here?


Bernd

Re: [Patch] Fix PR 60040

[Bernd Schmidt]

On 04/07/2016 01:52 PM, Senthil Kumar Selvaraj wrote:

   The below patch fixes PR 60040 by not halting with a hard error on
   a spill failure, if reload knows that it has to run again anyway.


Some additional information as to how this situation creates a spill 
failure would be useful. It's hard to tell whether this patch just 
papers over a problem that can still trigger in other circumstances.



-   spill_failure (chain->insn, rld[r].rclass);
-   failure = 1;
-   return;
+   if (!tentative)
+   {
+   spill_failure (chain->insn, rld[r].rclass);
+   failure = 1;
+   return;
+   }
  }


The indentation looks all wrong.


Bernd

Re: [PATCH] Add security_sensitive attribute to clean function stack and regs.

[Bernd Schmidt]

On 03/22/2016 03:15 PM, Marcos Díaz wrote:

the attached patch adds a new attribute 'security_sensitive' for functions.
The idea was discussed in PR middle-end/69976.


The first question would be: I see several submissions from folks 
@tallertechnologies.com. Do you and your employer have copyright 
assignments on file with the FSF?


The patch violates gcc coding guidelines in many ways. I'll point out 
some issues, but in general you are expected to familiarize yourself 
with the requirements first. Please review the entire patch yourself for 
such issues.



+bool ix86_sec_sensitive_attr_p(void)


Break line after return types.

  {
-  return TARGET_RED_ZONE && !TARGET_64BIT_MS_ABI;
+  return TARGET_RED_ZONE && !TARGET_64BIT_MS_ABI
+   && !ix86_sec_sensitive_attr_p();


Wrap multi-line expressions in parentheses to get correct formatting 
from the editor.



+  return (regno == BX_REG)
+|| (regno == BP_REG)
+|| (regno == SP_REG)
+|| (regno == R12_REG)
+|| (regno == R13_REG)
+|| (regno == R14_REG)
+|| (regno == R15_REG);


Same here but also remove unnecessary parentheses (in many places in 
this patch).



+}
+
+/* Returns true iff regno is an integer register.*/
+static inline bool is_integer_reg(unsigned int regno)


Space before open parens.


+  rtx ret = ix86_function_value
+   (TREE_TYPE (DECL_RESULT (cfun->decl)), cfun->decl, true);


Probably better to split this line after one of the commas.


+ // Is parallel


Use C style comments consistently.

+/* Adds to str in pos position the neame of the register regno.*/


Always two spaces after a period, including at the end of a comment.

In general it would be ideal if this functionality was not x86 specific. 
I could imagine that thread_prologue_and_epilogue_insns could do this in 
a machine-independent fashion.



Bernd

Re: [patch] Fix PR target/67172

[Bernd Schmidt]

On 04/04/2016 09:39 AM, Eric Botcazou wrote:

The audit trail of the PR is quite confused: __LIBGCC_EH_FRAME_SECTION_NAME__
needs to be (and is correctly) defined during the libgcc build because DWARF2
exceptions are used.  The problem is that libgcc2.c expects the usual non-ELF
setup, with __EH_FRAME_BEGIN__ defined in crtstuff.c.  Now t-cygwin has:


That's t-cygming, not t-cygwin, right?


This code is already entirely skipped for Cygwin because of:

#ifndef __CYGWIN__

so the attached patch adds a similar test on MinGW for the problematic code.

Tested on x86/Windows configured with --disable-sjlj-exceptions --with-dwarf2,
OK for the mainline and 5 branch?


It looks plausible. One could argue that gcc shouldn't define 
__LIBGCC_EH_FRAME_SECTION_NAME__ in this case, but since it's used in 
cygming-crtbegin.c, that seems to gain relatively little.


So, I think this is ok.


Bernd

Re: Fix for PR70498 in Libiberty Demangler

[Bernd Schmidt]

On 04/02/2016 11:49 AM, Marcel Böhme wrote:


Bootstrapped and regression tested on x86_64-pc-linux-gnu. Test cases
added + checked PR70498 is resolved.


Richard - any objections from an RM perspective to check in such
potentially security-related fixes now even if not regressions?


The patch now also accounts for overflows in d_compact_number which
is supposed to return -1 in case of negative numbers.


I take it this isn't for the normal 'n' case, but for instances where we 
encounter overflows in d_number?



Bernd

Re: Fix for PR70498 in Libiberty Demangler

[Bernd Schmidt]

On 04/01/2016 07:41 PM, Pedro Alves wrote:

On 04/01/2016 11:21 AM, Marcel Böhme wrote:

  static inline void
-d_append_num (struct d_print_info *dpi, long l)
+d_append_num (struct d_print_info *dpi, int l)
  {
char buf[25];
sprintf (buf,"%ld", l);


Doesn't this warn about %ld format vs int type mismatch?


Well spotted. Marcel, please correct this issue and check for other 
warnings. Unless libiberty is somehow exempt from -Werror, this should 
have shown up in a bootstrap.



Bernd

Re: [PATCH] Fix PR70484, RTL DSE using wrong dependence check

[Bernd Schmidt]

On 04/01/2016 11:08 AM, Richard Biener wrote:

{
! if (canon_true_dependence (s_info->mem,
!GET_MODE (s_info->mem),
!s_info->mem_addr,
!mem, mem_addr))
{
  s_info->rhs = NULL;
  s_info->const_rhs = NULL;
--- 1609,1617 
   the value of store_info.  If it is, set the rhs to NULL to
   keep it from being used to remove a load.  */
{
! if (canon_output_dependence (s_info->mem, true,
!  mem, GET_MODE (mem),
!  mem_addr))
{
  s_info->rhs = NULL;
  s_info->const_rhs = NULL;


I think the patch is ok, but there is a comment in that function which 
references canon_true_dependence; that should also be fixed up.


Isn't the testcase invalid though? I thought accesses through char * 
pointers bypass aliasing rules, but accessing a char array through int * 
and long * pointers doesn't?



Bernd

Re: [PATCH] Fix PR70484, RTL DSE using wrong dependence check

[Bernd Schmidt]

On 04/01/2016 05:05 PM, Jakub Jelinek wrote:

with my usual pair of rtl,yes checking bootstraps/regtests (x86_64-linux
and i686-linux, former one with ada, latter without), both without your
patch and with the patch.
Without the patch got 66555 successful replace_reads, with your patch
only 65971, that is 1% difference.  I guess that is acceptable level,
though for GCC 7 we should try to improve that (either at the GIMPLE level,
or do something better at the RTL level).


I was also running before/after tests on my set of .i files, and so far 
the effect seems negligible.



Bernd

Re: [PATCH] Improve CSE (PR rtl-optimization/70467)

[Bernd Schmidt]

On 04/01/2016 04:51 PM, Jakub Jelinek wrote:

On Fri, Apr 01, 2016 at 03:35:19PM +0200, Bernd Schmidt wrote:

On 04/01/2016 03:14 PM, Jakub Jelinek wrote:

As the testcase below shows, we can end up with lots of useless
instructions from multi-word arithmetics.
simplify-rtx.c can optimize x {&,|,^}= {0,-1}, but while
x &= 0 or x {|,^}= -1 are optimized into constants and CSE can handle those
fine, we keep x &= -1 and x {|,^}= 0 in the IL until expansion if x
is a MEM.  There are two issues, one is that cse_insn has for a few years
code that wants to prevent partially overlapping MEM->MEM moves,
but actually doesn't realize that fully overlapping MEM->MEM noop moves
are fine.  And the second one is that on most backends, there are no
MEM->MEM move instructions, so we need to delete the useless insns instead,
because it can't match.

Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux.
Is this something we want for 6.x or defer for stage1?


It seems like a stage1 thing to me unless it's a regression. But you're in a
better position to make that call.


I guess it can wait for stage1.


+ /* Similarly, lots of targets don't allow no-op
+(set (mem x) (mem x)) moves.  */
+ else if (n_sets == 1
+  && MEM_P (trial)
+  && MEM_P (dest)
+  && rtx_equal_p (trial, dest)
+  && !side_effects_p (dest)
+  && (cfun->can_delete_dead_exceptions
+  || insn_nothrow_p (insn)))


Looks like this block of code is practically duplicated - I'd prefer a
helper function set_of_equal_mems_removable_p or something. Ok with that
change.


Perhaps instead just set a bool in the second hunk and just test that at the
third hunk's condition?


Also works for me. Or maybe set trial and dest to pc_rtx and merge the 
new case with the preexisting one. As long as we don't get a big block 
of duplicated conditions.



Bernd

Re: Fix for PR70498 in Libiberty Demangler

[Bernd Schmidt]

On 04/01/2016 03:39 PM, Marcel Böhme wrote:

Hi Bernd,

Thanks for the feedback!


Patches need to be bootstrapped and regression tested, and patch
submissions should include which target this was done on.

Ideally you'd also want to include testcases along with your
patches, although I'm not entirely sure how we can arrange for this
type of problem to be tested.

Regression tested on x86_64-pc-linux-gnu (make check). Test cases
added to libiberty/testsuite/demangler-expected and checked PR70498
is resolved. Not sure how to bootstrap the patch.


You configure gcc normally and build it - that should automatically 
bootstrap, unless you're cross-compiling. You'll have stage1-* and 
stage2-* directories at the end if that worked. You should then run the 
testsuite on the bootstrapped compiler.



Lastly, for this specific patch, I have trouble seeing how it fixes
anything. I'd need a more detailed explanation of how the problem
happens in the first place.

In the patched version, the values wrap around when they are parsed
in d_number. Since the mangled string may contain negative numbers,
there is usually proper handling of negative numbers in the clients
of d_number. Without the patch a value can become negative when cast
from long to int *after* these checks.

For instance, in d_source_name the length of the identifier is parsed
as long from the mangled string and checked whether it is negative.
Since d_identifier takes an int as length, d_identifier is called
with a negative length after the implicit cast:


Ok, I think I see it. Guess I'll queue this up and commit it for you in 
the next few days.



Bernd

Re: Proposed Patch for Bug 69687

[Bernd Schmidt]

On 03/31/2016 06:56 AM, Marcel Böhme wrote:

Hi Bernd,


Are all the places being patched really problematic ones where an input file 
could realistically cause an overflow, or just the string functions?

The loop in demangle_args allows to call the patched register*- and 
remember*-methods arbitrarily often. So, those should also overflow at some 
point.
Found a few other segmentation faults in libiberty that I’ll report and patch 
separately.


I'm concerned about just returning without any kind of error indication. Not 
sure what we should be calling from libiberty, but I was thinking maybe 
xmalloc_failed.

Done. Now, clients of libiberty freeze for about 80 seconds and consume about 3GB of 
memory before exiting with "out of memory allocating 2147483647 bytes after a 
total of 3221147648 bytes”.


Might also want to guard against overflow from the first addition.

Done.




Index: libiberty/cplus-dem.c
===
--- libiberty/cplus-dem.c   (revision 234607)
+++ libiberty/cplus-dem.c   (working copy)
@@ -55,6 +55,7 @@ Boston, MA 02110-1301, USA.  */
  void * malloc ();
  void * realloc ();
  #endif
+#include 

  #include 
  #undef CURRENT_DEMANGLING_STYLE


Forgot about this issue, sorry. At least this needs guarding with #ifdef 
HAVE_LIMITS_H, as in the other files in libiberty. Several of them also 
go to trouble to define the macros if limits.h is missing; not sure how 
much of an issue that is nowadays, but you might want to adapt something 
like the code from strtol.c:


#ifndef ULONG_MAX
#define ULONG_MAX   ((unsigned long)(~0L))  /* 0x */
#endif

#ifndef LONG_MAX
#define LONG_MAX((long)(ULONG_MAX >> 1))/* 0x7FFF */
#endif

Mind trying that and doing a full test run as described in my other mail?


Bernd

Re: [PATCH] Improve CSE (PR rtl-optimization/70467)

[Bernd Schmidt]

On 04/01/2016 03:14 PM, Jakub Jelinek wrote:

As the testcase below shows, we can end up with lots of useless
instructions from multi-word arithmetics.
simplify-rtx.c can optimize x {&,|,^}= {0,-1}, but while
x &= 0 or x {|,^}= -1 are optimized into constants and CSE can handle those
fine, we keep x &= -1 and x {|,^}= 0 in the IL until expansion if x
is a MEM.  There are two issues, one is that cse_insn has for a few years
code that wants to prevent partially overlapping MEM->MEM moves,
but actually doesn't realize that fully overlapping MEM->MEM noop moves
are fine.  And the second one is that on most backends, there are no
MEM->MEM move instructions, so we need to delete the useless insns instead,
because it can't match.

Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux.
Is this something we want for 6.x or defer for stage1?


It seems like a stage1 thing to me unless it's a regression. But you're 
in a better position to make that call.



+ /* Similarly, lots of targets don't allow no-op
+(set (mem x) (mem x)) moves.  */
+ else if (n_sets == 1
+  && MEM_P (trial)
+  && MEM_P (dest)
+  && rtx_equal_p (trial, dest)
+  && !side_effects_p (dest)
+  && (cfun->can_delete_dead_exceptions
+  || insn_nothrow_p (insn)))


Looks like this block of code is practically duplicated - I'd prefer a 
helper function set_of_equal_mems_removable_p or something. Ok with that 
change.



Bernd

Re: Patches to fix optimizer bug & C++ exceptions for GCC VAX backend

[Bernd Schmidt]

Cc'ing Matt Thomas who is listed as the vax maintainer; most of the 
patch should be reviewed by him IMO. If he is no longer active I'd 
frankly rather deprecate the port rather than invest effort in keeping 
it running.



Index: gcc/except.c
===
RCS file: /cvsroot/src/external/gpl3/gcc/dist/gcc/except.c,v
retrieving revision 1.3
diff -u -r1.3 except.c
--- gcc/except.c23 Mar 2016 15:51:36 -  1.3
+++ gcc/except.c28 Mar 2016 23:24:40 -
@@ -2288,7 +2288,8 @@
  #endif
  {
  #ifdef EH_RETURN_HANDLER_RTX
-  emit_move_insn (EH_RETURN_HANDLER_RTX, crtl->eh.ehr_handler);
+  rtx insn = emit_move_insn (EH_RETURN_HANDLER_RTX, crtl->eh.ehr_handler);
+  RTX_FRAME_RELATED_P (insn) = 1;  // XXX FIXME in VAX backend?
  #else
error ("__builtin_eh_return not supported on this target");
  #endif


This part looks highly suspicious and I think there needs to be further 
analysis.



Bernd

Re: Fix for PR70498 in Libiberty Demangler

[Bernd Schmidt]

On 04/01/2016 12:21 PM, Marcel Böhme wrote:

This fixes the write access violation detailed in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70498 (and a few other
unreported cases).

Sometimes length-variables for strings and arrays are of type long
other times of type int. Since cp-demangle.h exports structs and
methods with length-variables of type int, this has been made
consistent in cp-demangle.c.


Patches need to be bootstrapped and regression tested, and patch 
submissions should include which target this was done on.


Ideally you'd also want to include testcases along with your patches, 
although I'm not entirely sure how we can arrange for this type of 
problem to be tested.


Lastly, for this specific patch, I have trouble seeing how it fixes 
anything. I'd need a more detailed explanation of how the problem 
happens in the first place.



Bernd

Re: [PATCH 1/2] Fix new -Wparentheses warnings encountered during bootstrap

[Bernd Schmidt]

On 03/31/2016 10:53 PM, Patrick Palka wrote:

This patch fixes the new -Wparentheses warnings (implemented by the
subsequent patch) that are encountered during bootstrap:

/home/patrick/code/gcc/gcc/omp-low.c: In function ‘void 
scan_sharing_clauses(tree, omp_context*, bool)’:
/home/patrick/code/gcc/gcc/omp-low.c:2381:6: error: suggest explicit braces to 
avoid ambiguous ‘else’ [-Werror=parentheses]
if (scan_array_reductions)
   ^
/home/patrick/code/gcc/gcc/gimplify.c: In function ‘gimple* 
gimplify_omp_ordered(tree, gimple_seq)’:
/home/patrick/code/gcc/gcc/gimplify.c:9880:6: error: suggest explicit braces to 
avoid ambiguous ‘else’ [-Werror=parentheses]
if (gimplify_omp_ctxp)
   ^
In file included from /home/patrick/code/gcc/gcc/cp/optimize.c:25:0:
/home/patrick/code/gcc/gcc/cp/optimize.c: In function ‘void 
populate_clone_array(tree, tree_node**)’:
/home/patrick/code/gcc/gcc/cp/cp-tree.h:2529:6: error: suggest explicit braces 
to avoid ambiguous ‘else’ [-Werror=parentheses]
if (TREE_CODE (FN) == FUNCTION_DECL   \
   ^
/home/patrick/code/gcc/gcc/cp/optimize.c:222:3: note: in expansion of macro 
‘FOR_EACH_CLONE’
FOR_EACH_CLONE (clone, fn)
^~
/home/patrick/code/gcc/gcc/fortran/openmp.c: In function ‘gfc_omp_udr* 
gfc_find_omp_udr(gfc_namespace*, const char*, gfc_typespec*)’:
/home/patrick/code/gcc/gcc/fortran/openmp.c:177:10: error: suggest explicit 
braces to avoid ambiguous ‘else’ [-Werror=parentheses]
if (st != NULL)
   ^

In each case I think the warning is harmless since the indentation of
the code in question corresponds to how the "else" is actually parsed
so I fixed each case simply by enclosing the entire body of the outer
"if" in braces.

The FOR_EACH_CLONE change resolves the cp/optimize.c warning.  It
adjusts the layout of the macro from

   if (p)
 for (...)

to

   if (!p)
 ;
   else for (...)

so that an "else" encountered in the body of the for-statement can no
longer possibly bind to the outer "if (p)" conditional.

Is this OK to commit after bootstrap + regtesting?


I think this is OK, now or in stage1 depending on whether the warning 
improvements go in now or later.


I see a patch for the C++ side fixing the warning, do you also intend to 
do C?



Bernd

Re: Fix for PR70481 Libiberty Demangler

[Bernd Schmidt]

On 03/31/2016 07:22 PM, Jeff Law wrote:

@@ -1237,11 +1237,13 @@  squangle_mop_up (struct work_stuff *work)

Thanks.  I've just installed this patch, along with suitable tests from
70481 and 67394.


What are the rules for modifying libiberty again? Do we have to patch 
binutils/gdb at the same time, or is there an automated process?



Bernd

Re: [PATCH] Fix ira.c indirect_jump_optimize (PR rtl-optimization/70460)

[Bernd Schmidt]

On 03/30/2016 11:27 PM, Jakub Jelinek wrote:

Hi!

As mentioned in the PR, we are miscompiling glibc on i686-linux, because
the new indirect_jump_optimize mini-pass thinks that a insn
which has REG_LABEL_OPERAND note necessarily has to set the target register
to that label, while in the glibc case it is actually that label + some
offset, where the offset is read from a table which contains other labels -
this label differences.

The following patch changes it to just look at SET_SRC of single_set and/or
REG_EQUAL note, and only consider those if one of them is a LABEL_REF.
That alone broke lots of tests, which contain non-local gotos, so I had
to add a check that we don't do anything in this mini-pass (like old ira.c
code did) if there is REG_NON_LOCAL_GOTO note.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/5/4.9?
Bernd has preapproved the patch, but that was before the REG_NON_LOCAL_GOTO
changes.


Still ok.


Bernd

Re: out of bounds access in insn-automata.c

[Bernd Schmidt]

On 03/25/2016 04:43 AM, Aldy Hernandez wrote:

If Bernd is fine with this, I'm happy to retract my patch and any
possible followups.  I'm just interested in having no path causing a
possible out of bounds access.  If your patch will do that, I'm cool.


I'll need to see that patch first to comment :-)


Bernd

Re: [PATCH] Fix num_imm_uses (PR tree-optimization/70405)

[Bernd Schmidt]

On 03/29/2016 07:28 PM, Jeff Law wrote:

On 03/29/2016 11:23 AM, Jakub Jelinek wrote:

Hi!

The recent change to num_imm_uses (to add support for NULL USE_STMT)
broke it totally, fortunately we have just one user of this function
right now.  I've filed a PR for GCC 7 so that we get a warning on this.

Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux,
ok for
trunk?

2016-03-29  Jakub Jelinek  

PR tree-optimization/70405
* ssa-iterators.h (num_imm_uses): Add missing braces.

* gcc.dg/pr70405.c: New test.

Not caught by -Wmisleading-indentation?  Seems like it'd be worth a bug
report for that.


Actually this looks like the dangling-else regression I've complained 
about previously. When I added that warning, I intentionally made it catch


if (foo)
  for (..)
if (bar)
...
else
  

but at some point the code was changed so as to no longer warn for this 
case.



Bernd

Re: Proposed Patch for Bug 69687

[Bernd Schmidt]

On 03/03/2016 03:55 PM, Marcel Böhme wrote:

@@ -4254,7 +4255,9 @@


Please use "diff -p" so that we get information about which function is 
being patched. Are all the places being patched really problematic ones 
where an input file could realistically cause an overflow, or just the 
string functions?



 }
else
 {
- work -> typevec_size *= 2;
+ if (work -> typevec_size > INT_MAX / 2)
+return;


I'm concerned about just returning without any kind of error indication. 
Not sure what we should be calling from libiberty, but I was thinking 
maybe xmalloc_failed.



@@ -4765,11 +4776,14 @@
  {
tem = s->p - s->b;
n += tem;
+  if ( n > INT_MAX / 2)
+return 0;
n *= 2;
s->b = XRESIZEVEC (char, s->b, n);
s->p = s->b + tem;
s->e = s->b + n;
  }


Might also want to guard against overflow from the first addition.


Bernd

Re: [DOC Patch] Add sample for @cc constraint

[Bernd Schmidt]

On 03/28/2016 12:03 AM, David Wohlferd wrote:

On 3/24/2016 8:00 AM, Bernd Schmidt wrote:
 > More problematic than a lack of documentation is that I haven't been
able to find an executable testcase. If you could adapt your example for
use in gcc.target/i386, that would be even more important.

It looks like Richard included some "scan-assembler" statements in the
suites with the original checkin
(https://gcc.gnu.org/viewcvs/gcc?view=revision=225122). Is that
not sufficient?  If not, I'm certainly prepared to create a couple
executable cases for the next rev of this patch.


I don't think it's sufficient. I would like executable code that 
verifies that this feature is indeed working as intended.



 > I don't think the manual should point out the obvious. I'd be
surprised if this wasn't documented or at least strongly implied
elsewhere for normal operands.

Well, *I* thought it was obvious, because it is both documented and
implied elsewhere.

However, the compiler doesn't see it that way.  Normally, attempting to
overlap 'clobbers' and 'outputs' generates compile errors, but not when
outputting and clobbering flags.  I filed pr68095 about this (including
a rough draft at a patch), but apparently not everyone sees this the way
I do.


Is there any _actual_ problem here? Like, if you combine the output and 
the clobber you run into problems? Looks to me like an explicit "cc" 
clobber is just ignored on x86. We just need to make sure this stays 
working (testcases).



 >> +Note: On the x86 platform, flags are normally considered clobbered by
 >> +extended asm whether the @code{"cc"} clobber is specified or not.
 >
 > Is it really necessary or helpful to mention that here? Not only is
it not strictly correct (an output operand is not also considered
clobbered), but to me it breaks the flow because you're left wondering
how that sentence relates to the example (it doesn't).

The problem I am trying to fix here is that on x86, the "cc" is implicit
for all extended asm statements, whether it is specified or not and
whether there is a flags output or not.  However, that fact isn't
documented anywhere.  So, where does that info go?  It could go right by
the docs for "cc", but since this behavior only applies to x86, that
would make the docs there messy.


My question would be, can this information ever be relevant to users? 
They may notice that their code still works if they omit the "cc", but 
that's not really a habit we want to encourage. I think this is an 
internal implementation detail that doesn't necessarily even have to be 
documented.



Bernd

Re: Goodbye REG_LIVE_LENGTH

[Bernd Schmidt]

On 03/25/2016 11:00 PM, Alan Modra wrote:

I'll also prepare a patch to delete REG_LIVE_LENGTH everywhere.


Like this.  Bootstrapped and regression tested x86_64-linux.
OK for stage1?


Oh wow that's a lot of stuff removed. Ok for this and the 
FREQ_CALLS_CROSSED patch.



Bernd

Re: [DOC Patch] Add sample for @cc constraint

[Bernd Schmidt]

In principle we probably should have an example, but once again I have 
some problems with the style of the added documentation. I prefer 
concise writing without unnecessary repetition. Any other reviewers can 
of course override me, but the following is my opinion on these changes.


More problematic than a lack of documentation is that I haven't been 
able to find an executable testcase. If you could adapt your example for 
use in gcc.target/i386, that would be even more important.


On 03/13/2016 05:00 AM, David Wohlferd wrote:

Index: extend.texi
===
--- extend.texi (revision 234163)
+++ extend.texi (working copy)
@@ -8047,6 +8047,7 @@

  Because of the special nature of the flag output operands, the constraint
  may not include alternatives.
+Do not clobber flags if they are being used as outputs.


I don't think the manual should point out the obvious. I'd be surprised 
if this wasn't documented or at least strongly implied elsewhere for 
normal operands.



+For builds that don't support flag output operands,


This feels strange, we should just be documenting the capabilities of 
this feature. Other parts of the docs already show what to do without 
it. Hence, reduce the example to this (plus the surrounding setup stuff):



+/* Avoid the redundant setc/testb and use the carry flag directly.  */
+asm ("bt $0, %1"
+  : "=@@ccc" (a)
+  : "r" (b));
+
+#endif



+Note: On the x86 platform, flags are normally considered clobbered by
+extended asm whether the @code{"cc"} clobber is specified or not.


Is it really necessary or helpful to mention that here? Not only is it 
not strictly correct (an output operand is not also considered 
clobbered), but to me it breaks the flow because you're left wondering 
how that sentence relates to the example (it doesn't).



  @anchor{InputOperands}
@@ -8260,6 +8298,8 @@
  On other machines, condition code handling is different,
  and specifying @code{"cc"} has no effect. But
  it is valid no matter what the target.
+For platform-specific uses of flags, see also
+@ref{FlagOutputOperands,Flag Output Operands}.


Is this likely to be helpful? Someone who's looking at how to use flag 
outputs probably isn't looking in the "Clobbers" section?



Bernd

Re: Fix 69650, bogus line numbers from libcpp

[Bernd Schmidt]

On 03/23/2016 03:21 PM, Richard Biener wrote:

On Wed, Mar 23, 2016 at 2:15 PM, Bernd Schmidt <bschm...@redhat.com> wrote:

On 03/23/2016 01:41 PM, Richard Biener wrote:


Btw, the issue in the PR is also fixed with a simple

Index: libcpp/line-map.c
===
--- libcpp/line-map.c   (revision 234415)
+++ libcpp/line-map.c   (working copy)
@@ -543,7 +543,7 @@ linemap_add (struct line_maps *set, enum
   to_file);

 /* A TO_FILE of NULL is special - we use the natural values.  */
-  if (error || to_file == NULL)
+  if (to_file == NULL)
  {
to_file = ORDINARY_MAP_FILE_NAME (from);
to_line = SOURCE_LINE (from, from[1].start_location);



I looked at that, but that made it hard to add the testcase as the line
numbers no longer match the dg-error directives. By moving this code we can
ignore the erroneous #line directive, and for this one testcase at least,
that makes the line numbers (and caret diagnostics etc.) come out right.


After some more digging and looking at your patch I'd approve that if it would
emit a warning rather than an error - so can you please adjust it?


Like this? No one has yet approved any better wording for the message, 
so given that you said "it's not a regression" I've left it, but I would 
now prefer "linemarker ignored due to incorrect nesting".



Bernd

	PR lto/69650
	* directives.c (do_linemarker): Test for file left but not entered
	here.
	* line-map.c (linemap_add): Not here.

	PR lto/69650
	* gcc.dg/pr69650.c: New test.

Index: gcc/testsuite/gcc.dg/pr69650.c
===
--- gcc/testsuite/gcc.dg/pr69650.c	(revision 0)
+++ gcc/testsuite/gcc.dg/pr69650.c	(working copy)
@@ -0,0 +1,5 @@
+/* { dg-do compile } */
+/* { dg-options "-std=gnu99" } */
+
+# 9 "somefile" 2 /* { dg-warning "left but not entered" } */
+not_a_type a; /* { dg-error "unknown type" } */
Index: libcpp/directives.c
===
--- libcpp/directives.c	(revision 234341)
+++ libcpp/directives.c	(working copy)
@@ -1046,6 +1046,19 @@
 
   skip_rest_of_line (pfile);
 
+  if (reason == LC_LEAVE)
+{
+  const line_map_ordinary *from;  
+  if (MAIN_FILE_P (map)
+	  || (new_file
+	  && (from = INCLUDED_FROM (pfile->line_table, map)) != NULL
+	  && filename_cmp (ORDINARY_MAP_FILE_NAME (from), new_file) != 0))
+	{
+	  cpp_warning (pfile, CPP_W_NONE,
+		 "file \"%s\" left but not entered", new_file);
+	  return;
+	}
+}
   /* Compensate for the increment in linemap_add that occurs in
  _cpp_do_file_change.  We're currently at the start of the line
  *following* the #line directive.  A separate source_location for this
Index: libcpp/line-map.c
===
--- libcpp/line-map.c	(revision 234341)
+++ libcpp/line-map.c	(working copy)
@@ -512,43 +512,23 @@
 	 "included", this variable points the map in use right before the
 	 #include "included", inside the same "includer" file.  */
   line_map_ordinary *from;
-  bool error;
 
-  if (MAIN_FILE_P (map - 1))
-	{
-	  /* So this _should_ mean we are leaving the main file --
-	 effectively ending the compilation unit. But to_file not
-	 being NULL means the caller thinks we are leaving to
-	 another file. This is an erroneous behaviour but we'll
-	 try to recover from it. Let's pretend we are not leaving
-	 the main file.  */
-	  error = true;
-  reason = LC_RENAME;
-  from = map - 1;
-	}
-  else
-	{
-	  /* (MAP - 1) points to the map we are leaving. The
-	 map from which (MAP - 1) got included should be the map
-	 that comes right before MAP in the same file.  */
-	  from = INCLUDED_FROM (set, map - 1);
-	  error = to_file && filename_cmp (ORDINARY_MAP_FILE_NAME (from),
-	   to_file);
-	}
+  linemap_assert (!MAIN_FILE_P (map - 1));
+  /* (MAP - 1) points to the map we are leaving. The
+	 map from which (MAP - 1) got included should be the map
+	 that comes right before MAP in the same file.  */
+  from = INCLUDED_FROM (set, map - 1);
 
-  /* Depending upon whether we are handling preprocessed input or
-	 not, this can be a user error or an ICE.  */
-  if (error)
-	fprintf (stderr, "line-map.c: file \"%s\" left but not entered\n",
-		 to_file);
-
   /* A TO_FILE of NULL is special - we use the natural values.  */
-  if (error || to_file == NULL)
+  if (to_file == NULL)
 	{
 	  to_file = ORDINARY_MAP_FILE_NAME (from);
 	  to_line = SOURCE_LINE (from, from[1].start_location);
 	  sysp = ORDINARY_MAP_IN_SYSTEM_HEADER_P (from);
 	}
+  else
+	linemap_assert (filename_cmp (ORDINARY_MAP_FILE_NAME (from),
+  to_file) == 0);
 }
 
   map->sysp = sysp;

Re: out of bounds access in insn-automata.c

[Bernd Schmidt]

On 03/24/2016 11:17 AM, Aldy Hernandez wrote:

On 03/23/2016 10:25 AM, Bernd Schmidt wrote:

It looks like this block of code is written by a helper function that is
really intended for other purposes than for maximal_insn_latency. Might
be worth changing to
  int insn_code = dfa_insn_code (as_a  (insn));
  gcc_assert (insn_code <= DFA__ADVANCE_CYCLE);


dfa_insn_code_* and friends can return > DFA__ADVANCE_CYCLE so I can't
put that assert on the helper function.


So don't use the helper function? Just emit the block above directly.


Bernd

Re: [patch] avoid double evaluation of PIC_OFFSET_TABLE_REGNUM

[Bernd Schmidt]

On 03/24/2016 11:32 AM, Aldy Hernandez wrote:

On x86, PIC_OFFSET_TABLE_REGNUM calls a function
(ix86_use_pseudo_pic_reg) so its value can theoretically change between
its first and second use in the following conditional:

if ((unsigned) PIC_OFFSET_TABLE_REGNUM != INVALID_REGNUM
   && fixed_regs[PIC_OFFSET_TABLE_REGNUM])

Since the macro can return -1 on x86, the second use can cause an out of
bounds access.

In practice ix86_use_pseudo_pic_reg() is probably a pure function, since
we really shouldn't be changing the semantics of the pic register
mid-flight, but it's probably safer to just avoid calling the function
twice.

OK pending tests?


Ok for stage 1.


Bernd

Re: Also test -O0 for OpenACC C, C++ offloading test cases

[Bernd Schmidt]

On 03/23/2016 07:47 AM, Thomas Schwinge wrote:

Want me to re-word that?  :-| I thought it would be obvious from looking
at the test case code; will not be a problem in practice.  It's because
of constructs used in the test cases, like the following, for example:



   if (__builtin_acc_on_device (5))
 {
   int g = 0, w = 0, v = 0;

   __asm__ volatile ("mov.u32 %0,%%ctaid.x;" : "=r" (g));
   __asm__ volatile ("mov.u32 %0,%%tid.y;" : "=r" (w));
   __asm__ volatile ("mov.u32 %0,%%tid.x;" : "=r" (v));
   ary[ix] = (g << 16) | (w << 8) | v;


Ok, maybe write "This code uses nvptx assembly guarded with 
__builtin_acc_on_device, which fails to be optimized away at -O0."


Ok with something like that.


Bernd

Re: Fix 69650, bogus line numbers from libcpp

[Bernd Schmidt]

On 03/23/2016 01:41 PM, Richard Biener wrote:

Btw, the issue in the PR is also fixed with a simple

Index: libcpp/line-map.c
===
--- libcpp/line-map.c   (revision 234415)
+++ libcpp/line-map.c   (working copy)
@@ -543,7 +543,7 @@ linemap_add (struct line_maps *set, enum
  to_file);

/* A TO_FILE of NULL is special - we use the natural values.  */
-  if (error || to_file == NULL)
+  if (to_file == NULL)
 {
   to_file = ORDINARY_MAP_FILE_NAME (from);
   to_line = SOURCE_LINE (from, from[1].start_location);


I looked at that, but that made it hard to add the testcase as the line 
numbers no longer match the dg-error directives. By moving this code we 
can ignore the erroneous #line directive, and for this one testcase at 
least, that makes the line numbers (and caret diagnostics etc.) come out 
right.



Bernd

Re: [PATCH 7/7] ira.c validate_equiv_mem

[Bernd Schmidt]

On 03/21/2016 02:43 AM, Alan Modra wrote:


+enum valid_equiv { valid_none, valid_combine, valid_reload };
+


Might be worth documenting that each step represents a superset of the 
previous one.



+ ret = valid_combine;
+ if (! MEM_READONLY_P (memref)
+ && ! RTL_CONST_OR_PURE_CALL_P (insn))
+   return valid_none;
+   }


The gcc style is actually not to have a space after unary "!". None of 
the code in this file follows that, but I think you may want to change 
that as you modify things in your patches, and have new code follow the 
recommended style.



@@ -3536,7 +3557,8 @@ update_equiv_regs (void)
{
  /* Note that the statement below does not affect the priority
 in local-alloc!  */
- REG_LIVE_LENGTH (regno) *= 2;
+ if (note)
+   REG_LIVE_LENGTH (regno) *= 2;


That's a very suspicious comment. It would be worth testing whether 
REG_LIVE_LENGTH has any effect on our current register allocation at 
all, and remove this code if not.


Otherwise looks good for stage 1.


Bernd

Re: Also test -O0 for OpenACC C, C++ offloading test cases

[Bernd Schmidt]

On 03/22/2016 11:23 AM, Thomas Schwinge wrote:

diff --git libgomp/testsuite/libgomp.oacc-c-c++-common/routine-w-1.c 
libgomp/testsuite/libgomp.oacc-c-c++-common/routine-w-1.c
index 01d1dc8..5806cb3 100644
--- libgomp/testsuite/libgomp.oacc-c-c++-common/routine-w-1.c
+++ libgomp/testsuite/libgomp.oacc-c-c++-common/routine-w-1.c
@@ -1,5 +1,6 @@
-/* { dg-do run } */
-/* { dg-additional-options "-O2" } */
+/* Dead code elimination for blocks guarded by acc_on_device () only works with
+   optimizations enabled.
+   { dg-skip-if "" { *-*-* } { "-O0" } { "" } } */


What exactly is going on with these? Do these tests fail with -O0, and 
is that likely to be a problem in practice?


Also, why remove the dg-do run?


Bernd

Re: [PATCH 6/7] ira.c use DF infrastructure for combine_and_move_insns

[Bernd Schmidt]

On 03/21/2016 02:42 AM, Alan Modra wrote:

* ira.c (combine_and_move_insns): Rather than scanning insns,
use DF infrastucture to find use and def insns.

- remove_death (regno, insn);


This call appears to have gone missing. Is that intentional?

Other than that it looks good for stage1.


Bernd

Re: Fix 70278 (LRA split_regs followup patch)

[Bernd Schmidt]

On 03/22/2016 10:24 AM, Christophe Lyon wrote:


The ARM test isn't sufficiently protected against non-compliant configurations,
and fails if GCC is configured for arm*linux-gnueabihf for instance
(see 
http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/234342/report-build-info.html)

The attached small patch fixes that by requiring arm_arch_v4t_multilib
effective target.

I used arm_arch_v4t_multilib instead of arm_arch_v4t because, as I
reported a long time ago
the later does not complain in some unsupported configuration because
the sample effective
target test does not contain actual code. In particular it's not
sufficient to reject thumb-1 with
hard-float.

OK?


No objections from me, but I copied all this from the existing testcase 
ftest-armv4t-thumb.c, so I'm puzzled why that one doesn't fail.



Bernd

Re: Fix 69650, bogus line numbers from libcpp

[Bernd Schmidt]

On 03/21/2016 09:15 PM, David Malcolm wrote:

On Mon, 2016-03-14 at 14:20 +0100, Bernd Schmidt wrote:

On 03/11/2016 11:09 PM, David Malcolm wrote:

+ cpp_error (pfile, CPP_DL_ERROR,
+"file \"%s\" left but not entered",
new_file);

   
Although it looks like you're preserving the existing behavior from
when this was in linemap_add, shouldn't this be
ORDINARY_MAP_FILE_NAME (from)
rather than new_file?  (i.e. shouldn't it report the name of the
file
being *left*, rather than the one being entered?)


I realize now that I was wrong here: "new_file" refers to the filename
given in the linemarker directive, which, depending on the (optional)
flag could be a directive to enter or leave a linemap.

Sorry about that; you may want to disregard my earlier worries.


No, I think you were mostly on point: new_file is the one in the #line 
directive, which AFAICT is the file being reentered. so the wording is 
in fact misleading. Including a file called "t.h" from "v.c" produces 
this pattern:


# 1 "t.h" 1
int t;
# 2 "v.c" 2


I was thinking of something like the attached, which makes things more
explicit; I felt the first dg-error in your patch was a little too
concise.


No problem, but I do think we want to change the wording of the message 
to something like "file %s unexpectedly reentered".



I'm very familiar with the code for tracking ranges and issuing
diagnostics, but less familiar with other parts of libcpp, so I'm not
sure I'm fully qualified to approve the patch.  That said, the patch
looks sane, mostly...  The remaining thing I have a concern about
relates to the other question I had, which I don't think you addressed:
is it possible to construct a testcase that triggers the logic in the
non-MAIN_FILE_P clause?


Are you thinking of anything more complex than the following?

# 1 "v.c"
# 1 "t.h" 1
# 1 "t2.h" 1
int t;
# 2 "t.h" 2
# 2 "v.c" 2

Change any of the filenames of the "^#.*2$" lines and you'll see error 
messages. For example, changing "t.h" to "x.h" in the second to last line:


In file included from t.h:1:0,
 from v.c:1:
t2.h:2:13: error: file "x.h" left but not entered
t2.h:3:12: error: file "v.c" left but not entered

Of course any such error is likely to have a large number of follow-on 
errors. Not sure how to avoid this given that the input file most likely 
has a completely messed up structure.



(How much existing test coverage do we have for line-markers?  I found
about 15 existing testcases that use them in a crude search with grep,
but these are all apparently only incidentally as part of testing other
functionality; is it worth me adding some more general test coverage
for this?)


It might be worthwhile but I'm not planning to for this issue.


Bernd

Re: [RFA][PATCH] Adding missing calls to bitmap_clear

[Bernd Schmidt]

On 03/21/2016 08:06 PM, Jeff Law wrote:


As noted last week, find_removable_extensions initializes several
bitmaps, but doesn't clear them.

This is not strictly a leak as the GC system should find dead data, but
it's better to go ahead and clear the bitmaps.  That releases the
elements back to the cache and presumably makes things easier for the GC
system as well.

Bootstrapped and regression tested on x86_64-linux-gnu.

OK for the trunk?


Looks like they don't leak anywhere, so ok. Probably ok even to install 
it now but maybe stage1 would be better timing.



Bernd

Re: Fix 69650, bogus line numbers from libcpp

[Bernd Schmidt]

Ping.


Bernd

On 03/14/2016 02:20 PM, Bernd Schmidt wrote:

On 03/11/2016 11:09 PM, David Malcolm wrote:

+  cpp_error (pfile, CPP_DL_ERROR,
+ "file \"%s\" left but not entered", new_file);

  
Although it looks like you're preserving the existing behavior from
when this was in linemap_add, shouldn't this be
   ORDINARY_MAP_FILE_NAME (from)
rather than new_file?  (i.e. shouldn't it report the name of the file
being *left*, rather than the one being entered?)


Hmm, almost but not quite. We don't necessarily know the name of the
file that's being left, if there's just a single #line directive as in
the testcase. I don't think we can reliably get a meaningful filename
other than the in the line directive. So maybe the error message needs
to be changed to something like "file %s unexpectedly reentered"?


Can we also have a testcase with a non-empty filename?  I'm interested
in seeing what the exact error messages looks like.


   # 1 "v.c"
   # 1 "t.h" 1
   int t;
   # 2 "v.c" 2

   int b;

t.h:2:12: error: file "b.c" left but not entered

So this shows the line number for the file we think we are in, which is
t.h. Would you accept this with the wording changed as suggested above?


Bernd

Re: [PATCH 0/7] ira.c tidies

[Bernd Schmidt]

On 03/21/2016 02:37 AM, Alan Modra wrote:

This series tidies some of the early ira code, in the process making a
tiny improvement to register pressure.  Patches 1 to 3 are fairly
simple tidies, with zero impact on generated code.  Patch 4 also is
mainly a tidy, but could see some extra REG_EQUIV notes added by
add_store_equivs.  In practice I didn't see any differences in gcc/*.o
on both x86_64 and powerpc64le (except ira.o of course) after this
patch.


These are ok when gcc-7 stage1 opens.


Patch 5 is also just a tidy.


This one I'm not sure I like. I see the benefit of using less memory, 
but it feels less robust to rely on "more or less valid" data, 
especially considering someone may add another optimization at some 
point without considering its effect on luids.


The other two I still need to look at a bit.


Bernd

Re: [PATCH, aarch64] Fix target/70120

[Bernd Schmidt]

On 03/17/2016 08:17 PM, Richard Henderson wrote:

With -g, and a code section that ends unaligned, the assembler complains of
"unaligned opcodes detected".  Except there are no such unaligned opcodes, nor
dwarf2 code ranges covering the end of the section, which arguably makes this
an assembler bug.  However, it's reasonably easy to work around in the
compiler, which saves having to bump the required binutils version.

Tested on aarch64-linux.


Ok for the varasm bits.


Bernd

Re: PING: [PATCH] PR driver/70192: Properly set flag_pie and flag_pic

[Bernd Schmidt]

On 03/17/2016 04:06 PM, H.J. Lu wrote:

This is the patch I am going to check in.


That still mentions darwin which I imagine might not be an exhaustive test.


Bernd

Fix 70278 (LRA split_regs followup patch)

[Bernd Schmidt]

This fixes an oversight in my previous patch here. I used biggest_mode 
in the assumption that if the reg was used in the function, it would be 
set to something other than VOIDmode, but that fails if we have a 
multiword access - only the first hard reg gets its biggest_mode 
assigned in that case.


Bootstrapped and tested on x86_64-linux, ran (just) the new arm testcase 
manually with arm-eabi. Ok?


(The testcase seems to be from glibc. Do we keep the copyright notices 
on the reduced form)?



Bernd
	PR rtl-optimization/70278
	* lra-constraints.c (split_reg): Handle the case where biggest_mode is
	VOIDmode.

testsuite/
	* gcc.dg/torture/pr70278.c: New test.
	* gcc.target/arm/pr70278.c: New test.

Index: gcc/lra-constraints.c
===
--- gcc/lra-constraints.c	(revision 234184)
+++ gcc/lra-constraints.c	(working copy)
@@ -4982,7 +4982,12 @@ split_reg (bool before_p, int original_r
   nregs = 1;
   mode = lra_reg_info[hard_regno].biggest_mode;
   machine_mode reg_rtx_mode = GET_MODE (regno_reg_rtx[hard_regno]);
-  if (GET_MODE_SIZE (mode) > GET_MODE_SIZE (reg_rtx_mode))
+  /* A reg can have a biggest_mode of VOIDmode if it was only ever seen
+	 as part of a multi-word register.  In that case, or if the biggest
+	 mode was larger than a register, just use the reg_rtx.  Otherwise,
+	 limit the size to that of the biggest access in the function.  */
+  if (mode == VOIDmode
+	  || GET_MODE_SIZE (mode) > GET_MODE_SIZE (reg_rtx_mode))
 	{
 	  original_reg = regno_reg_rtx[hard_regno];
 	  mode = reg_rtx_mode;
Index: gcc/testsuite/gcc.dg/torture/pr70278.c
===
--- gcc/testsuite/gcc.dg/torture/pr70278.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr70278.c	(working copy)
@@ -0,0 +1,37 @@
+/* { dg-do compile } */
+/*
+ * 
+ * Copyright (C) 1993 by Sun Microsystems, Inc. All rights reserved.
+ *
+ * Developed at SunPro, a Sun Microsystems, Inc. business.
+ * Permission to use, copy, modify, and distribute this
+ * software is freely granted, provided that this notice 
+ * is preserved.
+ * 
+ */
+typedef union
+{
+  double value;
+  struct
+  {
+unsigned int msw;
+  } parts;
+} ieee_double_shape_type;
+double __ieee754_hypot(double x, double y)
+{
+ double a=x,b=y,t1,t2,y1,y2,w;
+ int j,k,ha,hb;
+ do { ieee_double_shape_type gh_u; gh_u.value = (x); (ha) = gh_u.parts.msw; } while (0);;
+ if(hb > ha) {a=y;b=x;j=ha; ha=hb;hb=j;} else {a=x;b=y;}
+ if(ha > 0x5f30) {
+do { ieee_double_shape_type sh_u; sh_u.value = (a); sh_u.parts.msw = (ha); (a) = sh_u.value; } while (0);;
+ }
+ w = a-b;
+ if (w <= b)
+ {
+ t2 = a - t1;
+ w = t1*y1-(w*(-w)-(t1*y2+t2*b));
+ }
+ if(k!=0) {
+ } else return w;
+}
Index: gcc/testsuite/gcc.target/arm/pr70278.c
===
--- gcc/testsuite/gcc.target/arm/pr70278.c	(revision 0)
+++ gcc/testsuite/gcc.target/arm/pr70278.c	(working copy)
@@ -0,0 +1,41 @@
+/* { dg-do compile } */
+/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-march=*" } { "-march=armv4t" } } */
+/* { dg-skip-if "avoid conflicting multilib options" { *-*-* } { "-marm" } { "" } } */
+/* { dg-options "-mthumb" } */
+/* { dg-add-options arm_arch_v4t } */
+/*
+ * 
+ * Copyright (C) 1993 by Sun Microsystems, Inc. All rights reserved.
+ *
+ * Developed at SunPro, a Sun Microsystems, Inc. business.
+ * Permission to use, copy, modify, and distribute this
+ * software is freely granted, provided that this notice 
+ * is preserved.
+ * 
+ */
+typedef union
+{
+  double value;
+  struct
+  {
+unsigned int msw;
+  } parts;
+} ieee_double_shape_type;
+double __ieee754_hypot(double x, double y)
+{
+ double a=x,b=y,t1,t2,y1,y2,w;
+ int j,k,ha,hb;
+ do { ieee_double_shape_type gh_u; gh_u.value = (x); (ha) = gh_u.parts.msw; } while (0);;
+ if(hb > ha) {a=y;b=x;j=ha; ha=hb;hb=j;} else {a=x;b=y;}
+ if(ha > 0x5f30) {
+do { ieee_double_shape_type sh_u; sh_u.value = (a); sh_u.parts.msw = (ha); (a) = sh_u.value; } while (0);;
+ }
+ w = a-b;
+ if (w <= b)
+ {
+ t2 = a - t1;
+ w = t1*y1-(w*(-w)-(t1*y2+t2*b));
+ }
+ if(k!=0) {
+ } else return w;
+}

Re: [RFA][PR rtl-optimization/70263] Fix creation of new REG_EQUIV notes

[Bernd Schmidt]

On 03/18/2016 08:14 PM, Jeff Law wrote:

I also added a blurb to the dump file when we create these equivalences
and included a test to verify the code fires.  I verified it fired on
x86 and x86-64.  It may or may not fire on other targets, so I left the
test in the i386 specific subdirectory.


This is the sort of thing I'd want to do with rtl unit tests.


Bootstrapped and regression tested on x86_64-linux-gnu.  And unlike the
last version, it doesn't totally disable the note creation (as verified
by the new test ;-)

OK for the trunk?


Ok.


Bernd

Re: PING: [PATCH] PR driver/70192: Properly set flag_pie and flag_pic

[Bernd Schmidt]

On 03/17/2016 02:59 PM, H.J. Lu wrote:

On Fri, Mar 11, 2016 at 9:09 AM, H.J. Lu  wrote:

We can't set flag_pie to the default when flag_pic == 0, which may be
set by -fno-pic or -fno-PIC, since the default value of flag_pie is
non-zero when GCC is configured with --enable-default-pie.  We need
to initialize flag_pic to -1 so that we can tell if -fpic, -fPIC,
-fno-pic or -fno-PIC is used.



 PR driver/70192
 * opts.c (finish_options): Don't set flag_pie to the default if
 -fpic, -fPIC, -fno-pic or -fno-PIC is used.  Set flag_pic to 0
 if it is -1.


I think this part is ok.


diff --git a/gcc/testsuite/gcc.dg/pie-2.c b/gcc/testsuite/gcc.dg/pie-2.c
new file mode 100644
index 000..e185e51
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pie-2.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-fPIE" } */
+
+#if __PIC__ != 2
+# error __PIC__ is not 2!
+#endif
+
+#if __PIE__ != 2
+# error __PIE__ is not 2!
+#endif


In normal code that should probably use the "__PIC__ - 0" trick to guard 
against cases where the macro isn't defined, but I suppose we'd be 
getting an error in that case as well.



diff --git a/gcc/testsuite/gcc.dg/pie-3.c b/gcc/testsuite/gcc.dg/pie-3.c
new file mode 100644
index 000..fe46c98
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pie-3.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-fno-pie" } */
+
+#ifdef __PIC__
+# error __PIC__ is defined!
+#endif
+
+#ifdef __PIE__
+# error __PIE__ is defined!
+#endif
diff --git a/gcc/testsuite/gcc.dg/pie-4.c b/gcc/testsuite/gcc.dg/pie-4.c
new file mode 100644
index 000..977baf0
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pie-4.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-fno-PIE" } */
+
+#ifdef __PIC__
+# error __PIC__ is defined!
+#endif
+
+#ifdef __PIE__
+# error __PIE__ is defined!
+#endif

>> diff --git a/gcc/testsuite/gcc.dg/pie-6.c b/gcc/testsuite/gcc.dg/pie-6.c
>> new file mode 100644
>> index 000..85529a8
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/pie-6.c
>> @@ -0,0 +1,10 @@
>> +/* { dg-do compile { target { ! pie_enabled } } } */
>> +/* { dg-options "" } */
>> +
>> +#ifdef __PIC__
>> +# error __PIC__ is defined!
>> +#endif
>> +
>> +#ifdef __PIE__
>> +# error __PIE__ is defined!
>> +#endif

These I'm not so sure about. I could imagine there are targets where pic 
is the default. I'd remove these tests or the test for __PIC__. So, ok 
with that change.



Bernd

Re: PING: [PATCH] PR driver/70192: Properly set flag_pie and flag_pic

[Bernd Schmidt]

On 03/17/2016 04:13 PM, H.J. Lu wrote:

On Thu, Mar 17, 2016 at 8:09 AM, Bernd Schmidt <bschm...@redhat.com> wrote:

On 03/17/2016 04:06 PM, H.J. Lu wrote:


This is the patch I am going to check in.

That still mentions darwin which I imagine might not be an exhaustive test.


We can add an effective target, something like ignore_pic_pie, and
use it instead of *-*-darwin*.


That should have been done _before_ committing the patch in a form that 
was not approved.



Bernd

Re: PING: [PATCH] PR driver/70192: Properly set flag_pie and flag_pic

[Bernd Schmidt]

On 03/17/2016 04:26 PM, H.J. Lu wrote:

On Thu, Mar 17, 2016 at 8:23 AM, Bernd Schmidt <bschm...@redhat.com> wrote:

On 03/17/2016 04:13 PM, H.J. Lu wrote:

We can add an effective target, something like ignore_pic_pie, and
use it instead of *-*-darwin*.



That should have been done _before_ committing the patch in a form that was
not approved.



How should we move forward?


Maybe an effective target pic_default, which tests whether __PIC__ is 
defined without any options. Please prepare a patch.



Bernd

Re: RFA: PATCH to load_register_parameters for empty structs and sibcalls

[Bernd Schmidt]

On 03/16/2016 07:45 PM, Jason Merrill wrote:

Discussion of empty class parameter passing ABI led me to notice that
r162402 broke sibcalls with arguments of size 0 in some cases.  Before
that commit, the code read


else if ((partial == 0 || args[i].pass_on_stack)
 && size != 0)
{

[...]

  if (is_sibcall
  && mem_overlaps_already_clobbered_arg_p (XEXP (args[i].value,
0), size))
 *sibcall_failure = 1;


and after,



if (is_sibcall
&& (size == 0
|| mem_overlaps_already_clobbered_arg_p
 (XEXP (args[i].value, 0),
size)))



So now we set *sibcall_failure if size==0, whereas before we didn't
enter the outer block.  The comment also contradicts the code.


The patch looks ok. I was trying to research the earlier change, but I 
can't find a message in the archives. Cc'ing Iain in case he has input.



Bernd

Re: [PATCH] Change replace_rtx if from is a REG (PR target/70245, take 2)

[Bernd Schmidt]

On 03/17/2016 12:16 PM, Jakub Jelinek wrote:


Thus, I've reverted the patch (kept the testcase), and after some
discussions on IRC bootstrapped/regtested on x86_64-linux and i686-linux
following version, which right now should change behavior just for the i?86
case and nothing else, so shouldn't break other targets.
I believe at least the epiphany and sh peepholes that use replace_rtx will
want similar treatment, but will leave testing of that to their maintainers.

Ok for trunk?


Ok.


Bernd

Re: [PATCH] Change replace_rtx if from is a REG (PR target/70245)

[Bernd Schmidt]

On 03/16/2016 01:22 PM, Jakub Jelinek wrote:

So, this is what we've converged to on IRC and passed bootstrap/regtest
on x86_64-linux and i686-linux.  Is this ok for trunk?


The explanation was a bit confusing at first, but I think this looks 
reasonable. The assert worries me, but triggering it would indicate a 
potential miscompilation, so I think it is preferrable to have it.



Bernd

Re: [RFA][PR rtl-optimization/70263] Fix creation of new REG_EQUIV notes

[Bernd Schmidt]

On 03/17/2016 06:37 PM, Jeff Law wrote:

+  bitmap seen_insns;

+  seen_insns = BITMAP_ALLOC (NULL);


You could save an allocation here by making this a bitmap_head and using 
bitmap_initialize.



+  bitmap_set_bit (seen_insns, INSN_UID (insn));
+
   if (! INSN_P (insn))
continue;

@@ -3646,7 +3656,8 @@ update_equiv_regs (void)
  && ! find_reg_note (XEXP (reg_equiv[regno].init_insns, 0),
  REG_EQUIV, NULL_RTX)
  && ! contains_replace_regs (XEXP (dest, 0))
- && ! pdx_subregs[regno])
+ && ! pdx_subregs[regno]
+ && ! bitmap_bit_p (seen_insns, INSN_UID (insn)))


This looks odd to me. Isn't this condition always false? Did you want to 
test the init_insn?



Bernd

Re: Wonly-top-basic-asm

[Bernd Schmidt]

On 03/17/2016 06:23 AM, David Wohlferd wrote:

2016-03-16  David Wohlferd  <d...@limegreensocks.com>
     Bernd Schmidt  <bschm...@redhat.com>

 * doc/extend.texi: Doc basic asm behavior re clobbers.



Any objections from the release managers if I install this for David at 
this stage?



Bernd

Re: Wonly-top-basic-asm

[Bernd Schmidt]

On 03/17/2016 06:23 AM, David Wohlferd wrote:

On 3/14/2016 8:28 AM, Bernd Schmidt wrote:

The example is not good, as discussed previously, and IMO the best
option is to remove it. Otherwise I have no objections to the latest
variant.


Despite the problems I have with the existing sample, adding the
information/warnings is more important to me, and more valuable to
users.  Perhaps we can revisit the sample when pr24414 gets addressed.


My thought was to remove the example altogether, which we might want to 
do later on. But for now I've committed your change since it can be seen 
as an improvement in itself.



Bernd

Re: [PATCH] PR69195, Reload confused by invalid reg equivs

[Bernd Schmidt]

On 03/15/2016 03:27 AM, Alan Modra wrote:

On Mon, Mar 14, 2016 at 01:00:39PM -0600, Jeff Law wrote:

Right.  Tolerant as in not crash.


So can someone please approve my ira.c:indirect_jump_optimize patch?
I'm not quite audacious enough to claim it is obvious.


Looks good to me.


Bernd

Re: [PATCH] Fix combine's simplify_shift_const_1 (PR rtl-optimization/70222)

[Bernd Schmidt]

On 03/15/2016 12:14 PM, Jakub Jelinek wrote:

-  if (orig_code == LSHIFTRT && result_mode != shift_mode)
+ turn off all the bits that the shift would have turned off.
+ Similarly do this if we've optimized varop so that we don't perform
+ any shift.  */
+  if (orig_code == LSHIFTRT
+  && (result_mode != shift_mode
+ || (result_mode != mode
+ && count == 0
+ && orig_count != 0
+ && outer_op == UNKNOWN
+ && !complement_p)))


This looks really specialized, and I'd be worrying about whether it 
really is the right condition. Where exactly was the constant shifted by 
31 and count set to 0? Must be here, right?


   /* If we have (A << B << C) for any shift, we can convert this to
  (A << C << B).  This wins if A is a constant.  Only try this if
  B is not a constant.  */

   else if (GET_CODE (varop) == code
&& CONST_INT_P (XEXP (varop, 0))
&& !CONST_INT_P (XEXP (varop, 1)))
{
  rtx new_rtx = simplify_const_binary_operation (code, mode,
XEXP (varop, 0),
GEN_INT (count));
  varop = gen_rtx_fmt_ee (code, mode, new_rtx, XEXP (varop, 1));
  count = 0;
  continue;
}

I think it might be clearer to notice and fix the problem here (or set a 
need_mask flag).



Bernd

Re: [PATCH] genrecog: Fix crash on invalid input

[Bernd Schmidt]

On 03/14/2016 09:00 PM, Segher Boessenkool wrote:

There is just the single caller, and pred is set right before the call
there.  How about this patch, then?


Looks alright.


Bernd

Re: [PATCH] genrecog: Fix crash on invalid input

[Bernd Schmidt]

On 03/14/2016 04:38 PM, Segher Boessenkool wrote:

If your machine description refers to a non-existent predicate genrecog
crashes.  This fixes it.


Might be better to fix the caller?


Bernd

Re: [01/05] Fix PR 64411

[Bernd Schmidt]

On 03/14/2016 05:23 PM, Alexander Monakov wrote:

On Mon, 14 Mar 2016, Andrey Belevantsev wrote:

In this case, we get an inconsistency between the sched-deps interface, saying
we can't move an insn writing the si register through a vector insn, and the
liveness analysis, saying we can.  The latter doesn't take into account
implicit_reg_pending_clobbers set calculated in sched-deps before register
allocation.  The solution is to reflect this set in our insn data
(sets/uses/clobbers).

Ok for trunk?


One nit; the prototype of the new function:

extern void get_implicit_reg_pending_clobbers (rtx_insn *, HARD_REG_SET *);

has source operand on the left, destination on the right; it's probably nicer
to swap them around.

OK as far as selective scheduler changes go, but this also needs a general
scheduler maintainer ack for the sched-deps.c change.  Vladimir, can you have
a look?


Needs better documentation of the new function's arguments (as per 
general requirements for such things), but otherwise that part is ok 
(either arg order). The sel-sched parts should also have proper function 
comments however, and here:


+{
+  SET_REGNO_REG_SET (IDATA_REG_SETS (id), regno);
+}

we don't use braces around single statements.


Bernd

Re: Wonly-top-basic-asm

[Bernd Schmidt]

On 03/11/2016 01:55 AM, David Wohlferd wrote:

So, we have been discussing this issue for 4 months now.  Over that
time, I have tried to incorporate everyone's feedback.

As a result we have gone from a tiny doc patch (just describe the
current semantics), to a big doc patch (completely deprecate basic asm
when used in a function) to a medium doc patch + code fix (warning when
using basic asm in a function) and now back to a
slightly-bigger-than-tiny doc patch.

I have made no changes since the last patch I posted
(https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01406.html) for the
reasons discussed below.

I assert that this patch both contains important information users need
and is better than the current text.  I expect that Sandra is prepared
to check this in as soon as someone signs off on its technical accuracy.


The example is not good, as discussed previously, and IMO the best 
option is to remove it. Otherwise I have no objections to the latest 
variant.



Bernd

Re: Fix 69650, bogus line numbers from libcpp

[Bernd Schmidt]

On 03/11/2016 11:09 PM, David Malcolm wrote:

+ cpp_error (pfile, CPP_DL_ERROR,
+"file \"%s\" left but not entered", new_file);

  
Although it looks like you're preserving the existing behavior from
when this was in linemap_add, shouldn't this be
   ORDINARY_MAP_FILE_NAME (from)
rather than new_file?  (i.e. shouldn't it report the name of the file
being *left*, rather than the one being entered?)


Hmm, almost but not quite. We don't necessarily know the name of the 
file that's being left, if there's just a single #line directive as in 
the testcase. I don't think we can reliably get a meaningful filename 
other than the in the line directive. So maybe the error message needs 
to be changed to something like "file %s unexpectedly reentered"?



Can we also have a testcase with a non-empty filename?  I'm interested
in seeing what the exact error messages looks like.


  # 1 "v.c"
  # 1 "t.h" 1
  int t;
  # 2 "v.c" 2

  int b;

t.h:2:12: error: file "b.c" left but not entered

So this shows the line number for the file we think we are in, which is 
t.h. Would you accept this with the wording changed as suggested above?



Bernd

Re: [PATCH][ARM] PR driver/70132: Avoid double fclose in driver-arm.c

[Bernd Schmidt]

On 03/11/2016 04:32 PM, Kyrill Tkachov wrote:

 PR driver/70132
 * config/arm/driver-arm.c (host_detect_local_cpu): Set file pointer
 to NULL after closing file.


Doesn't match the patch. Either variant is fine but please use the right 
combination :)



Bernd

LRA remat issue with hard regs (PR70123)

[Bernd Schmidt]

When I submitted my previous lra-remat patch, I mentioned I had some 
concerns about the way we dealt with register number comparisons, but I 
didn't want to change things blindly without a testcase. PR70123 has now 
provided such a testcase where we are trying to rematerialize a hard 
register (r6). While scanning we encounter an instruction of the form

 (set (reg 285) (reg 272))
i.e. involving only pseudos, but reg_renumber[285] is r6. Since we only 
compare register numbers, we do not notice that the hard reg is clobbered.


The following patch modifies the function input_regno_present_p, and 
also renames it so that its purpose is more obvious to someone familiar 
with other parts of gcc. I've made it look at reg_renumber, and also try 
to deal with multi-word hard registers properly.


I'm not entirely sure this is a fully safe approach however, since I 
can't yet answer the question of whether LRA could change another pseudo 
to reside in hard register 6, thereby making the rematerialization 
invalid after the fact. Therefore the patch also includes a change to 
just disable candidates if they involve hard registers. I haven't 
observed that making any difference in code generation (on x86_64), 
beyond fixing the testcase on s390.


Bootstrapped and tested on x86_64-linux; Jakub verified that the 
testcase works afterwards. Ok for trunk and 5-branch, either for one or 
for both parts? I'm hoping the testcase in gcc.dg/torture will get 
exercised in the right way on s390, but I haven't run tests on that machine.



Bernd
	PR target/70123
	* lra-remat.c (operand_to_remat): Disallow hard regs in the value t
	be rematerialized.
	(reg_overlap_for_remat_p): Renamed from input_regno_present_p.
	Arguments swapped.  All callers changed.  Take reg_renumber into
	account, and Calculate and compare register ranges for hard regs.

	PR target/70123
	* gcc.dg/torture/pr70123.c: New test.

Index: gcc/lra-remat.c
===
--- gcc/lra-remat.c	(revision 234025)
+++ gcc/lra-remat.c	(working copy)
@@ -413,6 +413,10 @@ operand_to_remat (rtx_insn *insn)
   if (reg->regno >= FIRST_PSEUDO_REGISTER
 	  && bitmap_bit_p (_regs, reg->regno))
 	return -1;
+
+  /* Don't allow hard registers to be rematerialized.  */
+  if (reg->regno < FIRST_PSEUDO_REGISTER)
+	return -1;
 }
   if (found_reg == NULL)
 return -1;
@@ -718,21 +722,46 @@ calculate_local_reg_remat_bb_data (void)
 
 
 
-/* Return true if REGNO is an input operand of INSN.  */
+/* Return true if REG overlaps an input operand of INSN.  */
 static bool
-input_regno_present_p (rtx_insn *insn, int regno)
+reg_overlap_for_remat_p (lra_insn_reg *reg, rtx_insn *insn)
 {
   int iter;
   lra_insn_recog_data_t id = lra_get_insn_recog_data (insn);
   struct lra_static_insn_data *static_id = id->insn_static_data;
-  struct lra_insn_reg *reg;
-  
+  unsigned regno = reg->regno;
+  int nregs;
+
+  if (regno >= FIRST_PSEUDO_REGISTER && reg_renumber[regno] >= 0)
+regno = reg_renumber[regno];
+  if (regno >= FIRST_PSEUDO_REGISTER)
+nregs = 1;
+  else
+nregs = hard_regno_nregs[regno][reg->biggest_mode];
+
+  struct lra_insn_reg *reg2;
+
   for (iter = 0; iter < 2; iter++)
-for (reg = (iter == 0 ? id->regs : static_id->hard_regs);
-	 reg != NULL;
-	 reg = reg->next)
-  if (reg->type == OP_IN && reg->regno == regno)
-	return true;
+for (reg2 = (iter == 0 ? id->regs : static_id->hard_regs);
+	 reg2 != NULL;
+	 reg2 = reg2->next)
+  {
+	if (reg2->type != OP_IN)
+	  continue;
+	unsigned regno2 = reg2->regno;
+	int nregs2;
+
+	if (regno2 >= FIRST_PSEUDO_REGISTER && reg_renumber[regno2] >= 0)
+	  regno2 = reg_renumber[regno2];
+	if (regno >= FIRST_PSEUDO_REGISTER)
+	  nregs2 = 1;
+	else
+	  nregs2 = hard_regno_nregs[regno2][reg->biggest_mode];
+
+	if ((regno2 + nregs2 - 1 >= regno && regno2 < regno + nregs)
+	|| (regno + nregs - 1 >= regno2 && regno < regno2 + nregs2))
+	  return true;
+  }
   return false;
 }
 
@@ -833,7 +862,7 @@ calculate_gen_cands (void)
 			  && dst_regno == cand->regno)
 			continue;
 		  if (cand->regno == reg->regno
-			  || input_regno_present_p (insn2, reg->regno))
+			  || reg_overlap_for_remat_p (reg, insn2))
 			{
 			  bitmap_clear_bit (gen_cands, cand->index);
 			  bitmap_set_bit (_bitmap, uid);
@@ -1219,7 +1248,7 @@ do_remat (void)
 			&& dst_regno == cand->regno)
 		  continue;
 		if (cand->regno == reg->regno
-			|| input_regno_present_p (cand->insn, reg->regno))
+			|| reg_overlap_for_remat_p (reg, cand->insn))
 		  bitmap_set_bit (_bitmap, cand->index);
 		  }
 
Index: gcc/testsuite/gcc.dg/torture/pr70123.c
===
--- gcc/testsuite/gcc.dg/torture/pr70123.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr70123.c	(working copy)
@@ -0,0 +1,198 @@
+/* { dg-do run } */
+/* { dg-options "-fPIC" { target fpic } } */
+__attribute__ ((noinline, noclone)) int
+bar (int flag, 

Re: [PATCH] PR69195, Reload confused by invalid reg equivs

[Bernd Schmidt]

On 03/10/2016 10:18 AM, Alan Modra wrote:

Doing the indirect jump optimization turned out to be quite easy.

Bootstrapped and regression tested powerpc64le-linux, gcc-6, gcc-5 and
gcc-4.9.  Bootstrap and regression test x86_64-linux still running.
OK to apply?


So much nicer. Ok, and thanks.


Bernd

Fix 69650, bogus line numbers from libcpp

[Bernd Schmidt]

This is a case where bogus #line directives can confuse libcpp into 
producing nonsensical line numbers, even leading to a crash later on in LTO.


The following patch moves the test earlier to a point where we can more 
easily recover from the error condition. I should note that I changed 
the raw fprintf (stderr) to a cpp_error call, which is a slight change 
in behaviour (we don't even get to LTO anymore due to erroring out earlier).


Bootstrapped and tested on x86_64-linux (as always including Ada, which 
failed with an earlier version of the patch). Ok?



Bernd
	PR lto/69650
	* directives.c (do_linemarker): Test for file left but not entered
	here.
	* line-map.c (linemap_add): Not here.

	PR lto/69650
	* gcc.dg/pr69650.c: New test.

Index: libcpp/directives.c
===
--- libcpp/directives.c	(revision 234025)
+++ libcpp/directives.c	(working copy)
@@ -1046,6 +1046,19 @@ do_linemarker (cpp_reader *pfile)
 
   skip_rest_of_line (pfile);
 
+  if (reason == LC_LEAVE)
+{
+  const line_map_ordinary *from;  
+  if (MAIN_FILE_P (map)
+	  || (new_file
+	  && (from = INCLUDED_FROM (pfile->line_table, map)) != NULL
+	  && filename_cmp (ORDINARY_MAP_FILE_NAME (from), new_file) != 0))
+	{
+	  cpp_error (pfile, CPP_DL_ERROR,
+		 "file \"%s\" left but not entered", new_file);
+	  return;
+	}
+}
   /* Compensate for the increment in linemap_add that occurs in
  _cpp_do_file_change.  We're currently at the start of the line
  *following* the #line directive.  A separate source_location for this
Index: libcpp/line-map.c
===
--- libcpp/line-map.c	(revision 234025)
+++ libcpp/line-map.c	(working copy)
@@ -514,43 +514,23 @@ linemap_add (struct line_maps *set, enum
 	 "included", this variable points the map in use right before the
 	 #include "included", inside the same "includer" file.  */
   line_map_ordinary *from;
-  bool error;
-
-  if (MAIN_FILE_P (map - 1))
-	{
-	  /* So this _should_ mean we are leaving the main file --
-	 effectively ending the compilation unit. But to_file not
-	 being NULL means the caller thinks we are leaving to
-	 another file. This is an erroneous behaviour but we'll
-	 try to recover from it. Let's pretend we are not leaving
-	 the main file.  */
-	  error = true;
-  reason = LC_RENAME;
-  from = map - 1;
-	}
-  else
-	{
-	  /* (MAP - 1) points to the map we are leaving. The
-	 map from which (MAP - 1) got included should be the map
-	 that comes right before MAP in the same file.  */
-	  from = INCLUDED_FROM (set, map - 1);
-	  error = to_file && filename_cmp (ORDINARY_MAP_FILE_NAME (from),
-	   to_file);
-	}
 
-  /* Depending upon whether we are handling preprocessed input or
-	 not, this can be a user error or an ICE.  */
-  if (error)
-	fprintf (stderr, "line-map.c: file \"%s\" left but not entered\n",
-		 to_file);
+  linemap_assert (!MAIN_FILE_P (map - 1));
+  /* (MAP - 1) points to the map we are leaving. The
+	 map from which (MAP - 1) got included should be the map
+	 that comes right before MAP in the same file.  */
+  from = INCLUDED_FROM (set, map - 1);
 
   /* A TO_FILE of NULL is special - we use the natural values.  */
-  if (error || to_file == NULL)
+  if (to_file == NULL)
 	{
 	  to_file = ORDINARY_MAP_FILE_NAME (from);
 	  to_line = SOURCE_LINE (from, from[1].start_location);
 	  sysp = ORDINARY_MAP_IN_SYSTEM_HEADER_P (from);
 	}
+  else
+	linemap_assert (filename_cmp (ORDINARY_MAP_FILE_NAME (from),
+  to_file) == 0);
 }
 
   map->sysp = sysp;
Index: gcc/testsuite/gcc.dg/pr69650.c
===
--- gcc/testsuite/gcc.dg/pr69650.c	(revision 0)
+++ gcc/testsuite/gcc.dg/pr69650.c	(working copy)
@@ -0,0 +1,5 @@
+/* { dg-do compile } */
+/* { dg-options "-std=gnu99" } */
+
+# 9 "" 2 /* { dg-error "left but not entered" } */
+not_a_type a; /* { dg-error "unknown type" } */

Fix 70083, lra-induced crash

[Bernd Schmidt]

This crash happens because LRA tries to save an AVX hard reg in a large 
mode, and it only appears in the function in smaller modes. Stack 
alignment isn't set up to support the larger mode.


Currently, biggest_mode for hard registers is set up from regno_reg_rtx, 
set up to a large mode for argument regs. That mode is not necessarily 
seen in the function itself and may be too large. If that initialization 
is changed to use VOIDmode, we compute the correct value during 
lra_push_insns, but then subsequently we clear it to VOIDmode again, and 
it never seems to get updated. Hence, the patch has several parts: 
initialize hard reg biggest_mode with VOIDmode, ensure it gets updated 
during process_bb_lives, and use the value in split_reg.


Bootstrapped and tested on x86_64-linux, ok?


Bernd
	PR target/70083
	* lra-lives.c (process_bb_lives): Also update biggest mode for hard
	regs.
	(lra_create_live_ranges_1): initialize hard register biggest_mode to
	VOIDmode.
	* lra-constraints.c (split_reg): For hard regs, try to find the
	biggest single-register mode used in the function.

testsuite/
	PR target/70083
	* gcc.dg/torture/pr70083.c: New test.
	* gcc.target/i386/pr70083.c: New test.

Index: gcc/lra-lives.c
===
--- gcc/lra-lives.c	(revision 234025)
+++ gcc/lra-lives.c	(working copy)
@@ -700,12 +700,13 @@ process_bb_lives (basic_block bb, int 
 
   /* Update max ref width and hard reg usage.  */
   for (reg = curr_id->regs; reg != NULL; reg = reg->next)
-	if (reg->regno >= FIRST_PSEUDO_REGISTER
-	&& (GET_MODE_SIZE (reg->biggest_mode)
-		> GET_MODE_SIZE (lra_reg_info[reg->regno].biggest_mode)))
-	  lra_reg_info[reg->regno].biggest_mode = reg->biggest_mode;
-	else if (reg->regno < FIRST_PSEUDO_REGISTER)
-	  lra_hard_reg_usage[reg->regno] += freq;
+	{
+	  if (GET_MODE_SIZE (reg->biggest_mode)
+	  > GET_MODE_SIZE (lra_reg_info[reg->regno].biggest_mode))
+	lra_reg_info[reg->regno].biggest_mode = reg->biggest_mode;
+	  if (reg->regno < FIRST_PSEUDO_REGISTER)
+	lra_hard_reg_usage[reg->regno] += freq;
+	}
 
   call_p = CALL_P (curr_insn);
   src_regno = (set != NULL_RTX && REG_P (SET_SRC (set))
@@ -1208,7 +1209,7 @@ lra_create_live_ranges_1 (bool all_p, bo
 	 conservative because of recent transformation.  Here in this
 	 file we recalculate it again as it costs practically
 	 nothing.  */
-  if (regno_reg_rtx[i] != NULL_RTX)
+  if (i >= FIRST_PSEUDO_REGISTER && regno_reg_rtx[i] != NULL_RTX)
 	lra_reg_info[i].biggest_mode = GET_MODE (regno_reg_rtx[i]);
   else
 	lra_reg_info[i].biggest_mode = VOIDmode;
Index: gcc/lra-constraints.c
===
--- gcc/lra-constraints.c	(revision 234025)
+++ gcc/lra-constraints.c	(working copy)
@@ -4972,6 +4972,7 @@ split_reg (bool before_p, int original_r
   rtx_insn *restore, *save;
   bool after_p;
   bool call_save_p;
+  machine_mode mode;
 
   if (original_regno < FIRST_PSEUDO_REGISTER)
 {
@@ -4979,24 +4980,32 @@ split_reg (bool before_p, int original_r
   hard_regno = original_regno;
   call_save_p = false;
   nregs = 1;
+  mode = lra_reg_info[hard_regno].biggest_mode;
+  machine_mode reg_rtx_mode = GET_MODE (regno_reg_rtx[hard_regno]);
+  if (GET_MODE_SIZE (mode) > GET_MODE_SIZE (reg_rtx_mode))
+	{
+	  original_reg = regno_reg_rtx[hard_regno];
+	  mode = reg_rtx_mode;
+	}
+  else
+	original_reg = gen_rtx_REG (mode, hard_regno);
 }
   else
 {
+  mode = PSEUDO_REGNO_MODE (original_regno);
   hard_regno = reg_renumber[original_regno];
-  nregs = hard_regno_nregs[hard_regno][PSEUDO_REGNO_MODE (original_regno)];
+  nregs = hard_regno_nregs[hard_regno][mode];
   rclass = lra_get_allocno_class (original_regno);
   original_reg = regno_reg_rtx[original_regno];
   call_save_p = need_for_call_save_p (original_regno);
 }
-  original_reg = regno_reg_rtx[original_regno];
   lra_assert (hard_regno >= 0);
   if (lra_dump_file != NULL)
 fprintf (lra_dump_file,
 	 "	  \n");
+	  
   if (call_save_p)
 {
-  machine_mode mode = GET_MODE (original_reg);
-
   mode = HARD_REGNO_CALLER_SAVE_MODE (hard_regno,
 	  hard_regno_nregs[hard_regno][mode],
 	  mode);
@@ -5004,8 +5013,7 @@ split_reg (bool before_p, int original_r
 }
   else
 {
-  rclass = choose_split_class (rclass, hard_regno,
-   GET_MODE (original_reg));
+  rclass = choose_split_class (rclass, hard_regno, mode);
   if (rclass == NO_REGS)
 	{
 	  if (lra_dump_file != NULL)
@@ -5023,8 +5031,7 @@ split_reg (bool before_p, int original_r
 	}
 	  return false;
 	}
-  new_reg = lra_create_new_reg (GET_MODE (original_reg), original_reg,
-rclass, "split");
+  new_reg = lra_create_new_reg (mode, original_reg, rclass, "split");
   reg_renumber[REGNO (new_reg)] = hard_regno;
 }
   save = 

Re: Fix postreload_combine miscompilation (PR 69941)

[Bernd Schmidt]

On 03/05/2016 06:27 AM, Jeff Law wrote:

PR rtl-optimization/69941
* postreload.c (reload_combine_recognize_pattern): Ensure all uses of
the reg share its mode.

testsuite/
PR rtl-optimization/69941
* gcc.dg/torture/pr69941.c: New test.

OK.


For branches as well?


Bernd

Fix postreload_combine miscompilation (PR 69941)

[Bernd Schmidt]

This is a transformation which looks for

(set reg1 const)
(set reg2 (plus reg2 reg1))

and tries to replace all further uses of reg2 with (plus reg2 reg1). The 
problem here is that one of the uses is in a narrower mode, inside a 
zero_extend, so we produce wrong results.


The fix seems rather straightforward, verifying all uses have a mode 
matching the set. Bootstrapped and tested on x86_64-linux, ok?



Bernd
	PR rtl-optimization/69941
	* postreload.c (reload_combine_recognize_pattern): Ensure all uses of
	the reg share its mode.

testsuite/
	PR rtl-optimization/69941
	* gcc.dg/torture/pr69941.c: New test.

Index: gcc/postreload.c
===
--- gcc/postreload.c	(revision 233451)
+++ gcc/postreload.c	(working copy)
@@ -1057,7 +1057,6 @@ static bool
 reload_combine_recognize_pattern (rtx_insn *insn)
 {
   rtx set, reg, src;
-  unsigned int regno;
 
   set = single_set (insn);
   if (set == NULL_RTX)
@@ -1068,7 +1067,20 @@ reload_combine_recognize_pattern (rtx_in
   if (!REG_P (reg) || REG_NREGS (reg) != 1)
 return false;
 
-  regno = REGNO (reg);
+  unsigned int regno = REGNO (reg);
+  machine_mode mode = GET_MODE (reg);
+
+  if (reg_state[regno].use_index < 0
+  || reg_state[regno].use_index >= RELOAD_COMBINE_MAX_USES)
+return false;
+
+  for (int i = reg_state[regno].use_index;
+   i < RELOAD_COMBINE_MAX_USES; i++)
+{
+  struct reg_use *use = reg_state[regno].reg_use + i;
+  if (GET_MODE (*use->usep) != mode)
+	return false;
+}
 
   /* Look for (set (REGX) (CONST_INT))
  (set (REGX) (PLUS (REGX) (REGY)))
@@ -1090,8 +1102,6 @@ reload_combine_recognize_pattern (rtx_in
   && REG_P (XEXP (src, 1))
   && rtx_equal_p (XEXP (src, 0), reg)
   && !rtx_equal_p (XEXP (src, 1), reg)
-  && reg_state[regno].use_index >= 0
-  && reg_state[regno].use_index < RELOAD_COMBINE_MAX_USES
   && last_label_ruid < reg_state[regno].use_ruid)
 {
   rtx base = XEXP (src, 1);
Index: gcc/testsuite/gcc.dg/torture/pr69941.c
===
--- gcc/testsuite/gcc.dg/torture/pr69941.c	(revision 0)
+++ gcc/testsuite/gcc.dg/torture/pr69941.c	(working copy)
@@ -0,0 +1,30 @@
+/* { dg-do run } */
+ 
+int a = 0;
+int b = 0;
+int c = 0;
+int e = 0;
+int f = 0;
+int *g = 
+ 
+int fn1() { return b ? a : b; }
+ 
+int main() {
+  int h = fn1() <= 0x8000ULL; // h = 1;
+ 
+  int k = f; // k = 0;
+ 
+  long i = h ? k : k / h; // i = 0;
+ 
+  long l = (unsigned short)(i - 0x1800); // l = 0xe800
+ 
+  i = l ? l : c; // i = 0xe800;
+ 
+  *g = i; // *g = 0xe800; e = 0xe800;
+ 
+  unsigned char result = e >> 9; // result = 0x74;
+
+  if ((int)result != 0x74)
+__builtin_abort ();
+  return 0;
+}