[Bug target/69245] [6 Regression] ICE in extract_insn, at recog.c:2286 on arm-linux-gnueabihf

[ktkachov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69245

--- Comment #17 from ktkachov at gcc dot gnu.org ---
Author: ktkachov
Date: Fri Mar 11 15:27:24 2016
New Revision: 234141

URL: https://gcc.gnu.org/viewcvs?rev=234141=gcc=rev
Log:
[AArch64] PR target/70002: Make aarch64_set_current_function play nice with
pragma resetting

PR target/70002
* config/aarch64/aarch64-protos.h
(aarch64_save_restore_target_globals): New prototype.
* config/aarch64/aarch64-c.c (aarch64_pragma_target_parse):
Call the above when popping pragma.
* config/aarch64/aarch64.c (aarch64_save_restore_target_globals):
New function.
(aarch64_set_current_function): Rewrite using the above.

PR target/70002
PR target/69245
* gcc.target/aarch64/pr69245_2.c: New test.

Added:
trunk/gcc/testsuite/gcc.target/aarch64/pr69245_2.c
Modified:
trunk/gcc/ChangeLog
trunk/gcc/config/aarch64/aarch64-c.c
trunk/gcc/config/aarch64/aarch64-protos.h
trunk/gcc/config/aarch64/aarch64.c
trunk/gcc/testsuite/ChangeLog

[Bug target/69245] [6 Regression] ICE in extract_insn, at recog.c:2286 on arm-linux-gnueabihf

[ktkachov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69245

--- Comment #16 from ktkachov at gcc dot gnu.org ---
Author: ktkachov
Date: Fri Feb 26 16:02:21 2016
New Revision: 233745

URL: https://gcc.gnu.org/viewcvs?rev=233745=gcc=rev
Log:
[AArch64] Set TREE_TARGET_GLOBALS in aarch64_set_current_function when new tree
is the default node to recalculate optab availability

PR target/69245
* config/aarch64/aarch64.c (aarch64_set_current_function):
Save/restore target globals when switching to
target_option_default_node.

* gcc.target/aarch64/pr69245_1.c: New test.

Added:
trunk/gcc/testsuite/gcc.target/aarch64/pr69245_1.c
Modified:
trunk/gcc/ChangeLog
trunk/gcc/config/aarch64/aarch64.c
trunk/gcc/testsuite/ChangeLog

[Bug target/69245] [6 Regression] ICE in extract_insn, at recog.c:2286 on arm-linux-gnueabihf

[chrbr at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69245

--- Comment #14 from chrbr at gcc dot gnu.org ---
Author: chrbr
Date: Wed Jan 27 13:03:45 2016
New Revision: 232872

URL: https://gcc.gnu.org/viewcvs?rev=232872=gcc=rev
Log:
2016-01-20  Christian Bruel  

PR target/69245
* config/arm/arm-c.c (arm_pragma_target_parse): Add comments.
Move arm_reset_previous_fndecl and set_target_option_current_node in
the conditional part.  Call save_restore_target_globals.
* config/arm/arm.c (arm_set_current_function):
Refactor to better support #pragma target and attribute mix.
Call save_restore_target_globals.
* config/arm/arm-protos.h (save_restore_target_globals): New function.


Added:
trunk/gcc/testsuite/gcc.target/arm/pr69245.c
Modified:
trunk/gcc/ChangeLog
trunk/gcc/config/arm/arm-c.c
trunk/gcc/config/arm/arm-protos.h
trunk/gcc/config/arm/arm.c
trunk/gcc/testsuite/ChangeLog

[Bug target/69245] [6 Regression] ICE in extract_insn, at recog.c:2286 on arm-linux-gnueabihf

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69245

Jakub Jelinek  changed:

   What|Removed |Added

 Status|NEW |RESOLVED
 Resolution|--- |FIXED

--- Comment #15 from Jakub Jelinek  ---
Assuming fixed.

[Bug target/69245] [6 Regression] ICE in extract_insn, at recog.c:2286 on arm-linux-gnueabihf

[jgreenhalgh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69245

--- Comment #13 from James Greenhalgh  ---
This is a similar case I reduced from the Ubuntu rebuild failures, hitting the
"max" idiom recognition:

---

#pragma GCC push_options
#pragma GCC target("fpu=crypto-neon-fp-armv8")
static void
foo (void)
{
}
#pragma GCC pop_options
float
max_idiom (float x, float y)
{
  return x > y ? x : y;
}

---

Compile with -march=armv7-a -mfloat-abi=hard -mfpu=neon -O2 -ffast-math

Presumably the same root cause.

bug.c:12:1: error: unrecognizable insn:
 }
 ^

(insn 7 4 8 2 (set (reg:SF 113)
(smax:SF (reg/v:SF 112 [ y ])
(reg/v:SF 111 [ x ]))) bug2.c:11 -1
 (nil))

bug.c:12:1: internal compiler error: in extract_insn, at recog.c:2286
0xa411e9 _fatal_insn(char const*, rtx_def const*, char const*, int, char
const*)
.../gcc/rtl-error.c:108
0xa41208 _fatal_insn_not_found(rtx_def const*, char const*, int, char const*)
.../gcc/rtl-error.c:116
0xa1404d extract_insn(rtx_insn*)
.../gcc/recog.c:2286
0x82b664 instantiate_virtual_regs_in_insn
.../gcc/function.c:1582
0x82b664 instantiate_virtual_regs
.../gcc/function.c:1950
0x82b664 execute
.../gcc/function.c:1999
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See  for instructions.

[Bug target/69245] [6 Regression] ICE in extract_insn, at recog.c:2286 on arm-linux-gnueabihf

[chrbr at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69245

chrbr at gcc dot gnu.org changed:

   What|Removed |Added

   Assignee|unassigned at gcc dot gnu.org  |chrbr at gcc dot gnu.org

--- Comment #12 from chrbr at gcc dot gnu.org ---
Created attachment 37357
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=37357=edit
new arm_set_current_function implementation

A complete refactoring of arm_set_current_function. Much simpler. 

Also fixes pr68896 due to discrepancies between #pragma GCC target and
set_current_function global_options cl_target_option_restore restores.

Still want to play a little bit with it before sending it to the list.

[Bug target/69245] [6 Regression] ICE in extract_insn, at recog.c:2286 on arm-linux-gnueabihf

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69245

Richard Biener  changed:

   What|Removed |Added

   Priority|P3  |P1

[Bug target/69245] [6 Regression] ICE in extract_insn, at recog.c:2286 on arm-linux-gnueabihf

[chrbr at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69245

--- Comment #8 from chrbr at gcc dot gnu.org ---
Probably the same latent bug in the else part of the pragma_parse, for the case
where arm_valid_target_attribute_tree returns NULL.

[Bug target/69245] [6 Regression] ICE in extract_insn, at recog.c:2286 on arm-linux-gnueabihf

[chrbr at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69245

--- Comment #7 from chrbr at gcc dot gnu.org ---
also it could be wrong to wait for arm_set_current_function when handling a
pragma GCC target because we could need the state in the global scope.

[Bug target/69245] [6 Regression] ICE in extract_insn, at recog.c:2286 on arm-linux-gnueabihf

[chrbr at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69245

--- Comment #9 from chrbr at gcc dot gnu.org ---
Created attachment 37329
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=37329=edit
factorize restoration code

works for this case but fully untested

[Bug target/69245] [6 Regression] ICE in extract_insn, at recog.c:2286 on arm-linux-gnueabihf

[chrbr at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69245

--- Comment #6 from chrbr at gcc dot gnu.org ---
(In reply to ktkachov from comment #5)
> The problem is we never end up setting TREE_TARGET_GLOBALS for fn2.
> From what I can gather that's TARGET_SET_CURRENT_FUNCTION's job (although
> the documentation for these things is scarce :( )
> 
> When we call arm_set_current_function for fn2 the old_tree is NULL because
> arm_pragma_target_parse will have reset arm_previous_fndecl when popping the
> target pragma.
> 
> new_tree will also be NULL because DECL_FUNCTION_SPECIFIC_TARGET for fndecl
> is only supposed to be set in the target attribute parsing code, but not the
> pragma parsing code (again, documentation here is scarce).
> 
> So we end up bypassing all the content in arm_set_current_function and not
> setting TREE_TARGET_GLOBALS which means the midend doesn't try to reset the
> optabs for fn2.
> 
> x86 gets away with this because ix86_reset_previous_fndecl besides clearing
> the previous fndecl cache also sets TREE_TARGET_GLOBALS for
> target_option_current_node.
> 
> rs6000 seems to never reset its previous fndecl (rs6000_previous_fndecl) so
> its version old_tree will always point to some tree (AFAICT).
> 
> Ugh. So for arm my thoughts are we should not exit early on the old_tree ==
> new_tree condition if both trees are NULL but rather follow the second arm
> of the if statement in arm_set_current_function where we set new_tree to
> target_option_current_code.
> 
> Hacking a prototype for that idea fixes the testcase for me.
> Christian, what do you think?

arm_reset_current_function assumes indeed that all the state switches are done,
so it would be more consistent to call restore_target_globals in
pragma_target_parse as well instead of splitting the work in 2 different
places. Maybe factorizing the common code from both ?

[Bug target/69245] [6 Regression] ICE in extract_insn, at recog.c:2286 on arm-linux-gnueabihf

[chrbr at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69245

chrbr at gcc dot gnu.org changed:

   What|Removed |Added

  Attachment #37329|0   |1
is obsolete||

--- Comment #10 from chrbr at gcc dot gnu.org ---
Created attachment 37332
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=37332=edit
factorize restoration code

[Bug target/69245] [6 Regression] ICE in extract_insn, at recog.c:2286 on arm-linux-gnueabihf

[chrbr at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69245

chrbr at gcc dot gnu.org changed:

   What|Removed |Added

  Attachment #37332|0   |1
is obsolete||

--- Comment #11 from chrbr at gcc dot gnu.org ---
Created attachment 37333
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=37333=edit
factorize restoration code

better with the patch

[Bug target/69245] [6 Regression] ICE in extract_insn, at recog.c:2286 on arm-linux-gnueabihf

[ktkachov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69245

ktkachov at gcc dot gnu.org changed:

   What|Removed |Added

 Status|UNCONFIRMED |NEW
   Last reconfirmed||2016-01-12
   Target Milestone|--- |6.0
 Ever confirmed|0   |1

--- Comment #1 from ktkachov at gcc dot gnu.org ---
Confirmed.

[Bug target/69245] [6 Regression] ICE in extract_insn, at recog.c:2286 on arm-linux-gnueabihf

[ktkachov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69245

ktkachov at gcc dot gnu.org changed:

   What|Removed |Added

 CC||chrbr at gcc dot gnu.org

--- Comment #2 from ktkachov at gcc dot gnu.org ---
So the fpu given on the command line does not support vfma but the global
variables are enclosed in an fpu pragma that does.
But the fma itself occurs outside the scope of fpu=crypto-neon-fp-armv8.

More C-friendly testcase:
#pragma GCC push_options
#pragma GCC target "fpu=crypto-neon-fp-armv8"
int a, c, d;
float b;
static int fn1 ()
{
  return 0;
}

#pragma GCC pop_options
void fn2 ()
{
  d = b * c + a;
}

interestingly, deleting the definition of fn1 we don't ICE.

[Bug target/69245] [6 Regression] ICE in extract_insn, at recog.c:2286 on arm-linux-gnueabihf

[ktkachov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69245

--- Comment #3 from ktkachov at gcc dot gnu.org ---
Hmm...
The code in tree-ssa-math-opts.c that creates the FMA_EXPR avoids doing it if
the target doesn't support an fma optab.
So the optab handler should have rejected it.
The arm fma optab in vfp.md has the condition TARGET_32BIT && TARGET_HARD_FLOAT
&& TARGET_FMA.

TARGET_FMA is defined as (TARGET_VFP && TARGET_FPU_REV >= 4)
and TARGET_FPU_REV is (all_fpus[arm_fpu_index].rev)

at the point in convert_mult_to_fma in fn2 when the FMA_EXPR is created if I
print in gdb the expression:
all_fpus[global_options.x_arm_fpu_index].rev
I get '3'. So TARGET_FMA should be properly set to 0 at this point.
Something's weird going on.

[Bug target/69245] [6 Regression] ICE in extract_insn, at recog.c:2286 on arm-linux-gnueabihf

[ktkachov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69245

--- Comment #5 from ktkachov at gcc dot gnu.org ---
The problem is we never end up setting TREE_TARGET_GLOBALS for fn2.
>From what I can gather that's TARGET_SET_CURRENT_FUNCTION's job (although the
documentation for these things is scarce :( )

When we call arm_set_current_function for fn2 the old_tree is NULL because
arm_pragma_target_parse will have reset arm_previous_fndecl when popping the
target pragma.

new_tree will also be NULL because DECL_FUNCTION_SPECIFIC_TARGET for fndecl is
only supposed to be set in the target attribute parsing code, but not the
pragma parsing code (again, documentation here is scarce).

So we end up bypassing all the content in arm_set_current_function and not
setting TREE_TARGET_GLOBALS which means the midend doesn't try to reset the
optabs for fn2.

x86 gets away with this because ix86_reset_previous_fndecl besides clearing the
previous fndecl cache also sets TREE_TARGET_GLOBALS for
target_option_current_node.

rs6000 seems to never reset its previous fndecl (rs6000_previous_fndecl) so its
version old_tree will always point to some tree (AFAICT).

Ugh. So for arm my thoughts are we should not exit early on the old_tree ==
new_tree condition if both trees are NULL but rather follow the second arm of
the if statement in arm_set_current_function where we set new_tree to
target_option_current_code.

Hacking a prototype for that idea fixes the testcase for me.
Christian, what do you think?

[Bug target/69245] [6 Regression] ICE in extract_insn, at recog.c:2286 on arm-linux-gnueabihf

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69245

Jakub Jelinek  changed:

   What|Removed |Added

 CC||jakub at gcc dot gnu.org

--- Comment #4 from Jakub Jelinek  ---
I'd recommend writing similar testcase for x86_64 with #pragma GCC target "fma"
instead and debug side by side what is different in the target pragma handling.