On 07/30/2016 02:17 AM, Bernd Edlinger wrote:
In your first mail you showed reg 481 as _not_ being REG_POINTER:
(insn 1047 1046 1048 (set (reg:DI 481)
(subreg:DI (reg/f:SI 479) 0)) y.c:12702 -1
(nil))
(note the lack of /f). So which is it? REG_POINTER here is not correct
as far as I can see.
Oh yes, that's an interesting point, in expand I still see this:
(insn 1047 1046 1048 (set (reg:DI 481)
(subreg:DI (reg/f:SI 479) 0)) isl_input.c:2496 -1
(nil))
But in the last dump before combine I have this:
(insn 1047 1044 1048 101 (set (reg/f:DI 481)
(subreg:DI (reg/f:SI 545) 0)) isl_input.c:2496 50 {*movdi_aarch64}
(nil))
However I was not really surpised by that, because the reg 545 does
in deed hold a pointer value: &isl_obj_map_vtable
So just an FYI. It should always be safe to fail to mark something with
REG_POINTER -- though it is possible that something has violated that
design decision.
So one interesting test would be to hack up things so that REG_POINTER
never gets set on anything and see what that does to your testcase.
(insn 22 17 23 51 (set (reg/f:SI 544)
(high:SI (symbol_ref:SI ("isl_obj_map_vtable") [flags 0xc0]
<var_decl 0x7f83d3273f30 isl_obj_map_vtable>))) isl_input.c:2415 49
{*movsi_aarch64}
(nil))
(insn 23 22 24 51 (set (reg/f:SI 545)
(lo_sum:SI (reg/f:SI 544)
(symbol_ref:SI ("isl_obj_map_vtable") [flags 0xc0]
<var_decl 0x7f83d3273f30 isl_obj_map_vtable>))) isl_input.c:2415 917
{add_losym_si}
(expr_list:REG_DEAD (reg/f:SI 544)
(expr_list:REG_EQUAL (symbol_ref:SI ("isl_obj_map_vtable")
[flags 0xc0] <var_decl 0x7f83d3273f30 isl_obj_map_vtable>)
(nil))))
The "reg/f:DI 481" first appeared in cse1.
I'll try to see what's happening there next....
Ok, I the incorrect REG_POINTER is done here:
cse_main -> reg_scan -> reg_scan_mark_refs -> set_reg_attrs_from_value
(reg 544) is technically not a pointer, though I think we have allowed
REG_POINTER to be set on (HIGH (SYMBOL_REF)). I would expect (reg 545)
to be marked as a pointer.
The hesitation I have is because Pmode != SImode on this target, so
technically the value has to be zero extended out to Pmode to ensure its
validity. One could argue that only a properly extended object should
have REG_POINTER set.
and here I see a bug, because if POINTERS_EXTEND_UNSIGNED
can_be_reg_pointer is only set to false if x is SIGN_EXTEND but not
if x is a SUBREG as in this case.
Seems like a bug to me.
So I think that should be fixed this way:
Index: emit-rtl.c
===================================================================
--- emit-rtl.c (revision 238891)
+++ emit-rtl.c (working copy)
@@ -1155,7 +1155,7 @@ set_reg_attrs_from_value (rtx reg, rtx x)
|| (GET_CODE (x) == SUBREG && subreg_lowpart_p (x)))
{
#if defined(POINTERS_EXTEND_UNSIGNED)
- if (((GET_CODE (x) == SIGN_EXTEND && POINTERS_EXTEND_UNSIGNED)
+ if (((GET_CODE (x) != ZERO_EXTEND && POINTERS_EXTEND_UNSIGNED)
|| (GET_CODE (x) != SIGN_EXTEND && ! POINTERS_EXTEND_UNSIGNED))
&& !targetm.have_ptr_extend ())
can_be_reg_pointer = false;
What do you think does this look like the right fix?
As Segher pointed out, I think you also want to look at
SUBREG_PROMOTED_VAR_P and SUBREG_PROMOTED_UNSIGNED_P as well. I don't
think they're applicable for this target, but it still seems like the
right thing to do.
With this patch the code the reg/f:DI 481 does no longer appear,
and also the invalid combine does no longer happen.
However the test case from pr70903 does not get fixed by this.
But when I look at the dumps, I see again the first incorrect
transformation in cse2 (again cse!):
(insn 20 19 21 2 (set (reg:DI 94)
(subreg:DI (reg:QI 93) 0)) pr.c:8 50 {*movdi_aarch64}
(expr_list:REG_EQUAL (const_int 255 [0xff])
(expr_list:REG_DEAD (reg:QI 93)
(nil))))
but that is simply wrong, because later optimization passes
expect reg 94 to be 0x000000ff but the upper bits are unspecified,
so that REG_EQUAL should better not exist.
Now this one could be related to PROMOTE_MODE and friends. You might
want to review Jim W's comments in pr65932 which describe some problems
with the way the port uses PROMOTE_MODE.
When I looked at cse.c I saw a comment in #if 0, which exactly
describes the problem that we have with paradoxical subreg here:
Index: cse.c
===================================================================
--- cse.c (revision 238891)
+++ cse.c (working copy)
@@ -4716,10 +4716,6 @@ cse_insn (rtx_insn *insn)
}
}
-#if 0
- /* It is no longer clear why we used to do this, but it doesn't
- appear to still be needed. So let's try without it since this
- code hurts cse'ing widened ops. */
/* If source is a paradoxical subreg (such as QI treated as an SI),
treat it as volatile. It may do the work of an SI in one context
where the extra bits are not being used, but cannot replace an SI
@@ -4726,7 +4722,6 @@ cse_insn (rtx_insn *insn)
in general. */
if (paradoxical_subreg_p (src))
sets[i].src_volatile = 1;
-#endif
/* Locate all possible equivalent forms for SRC. Try to replace
SRC in the insn with each cheaper equivalent.
removing the #if 0 seems to fix the sample from pr70903, but generates
3 more instructions than with my initial patch:
Right. I'd have to look at cse for a while, but ISTM that treating it
like it was volatile is more of a pessimization than we want/need.
When I looked at svn blame cse.c, I saw that this #if 0 was introduced
here:
svn log -r1614 -v
------------------------------------------------------------------------
r1614 | kenner | 1992-07-17 11:57:24 +0200 (Fri, 17 Jul 1992) | 2 lines
Changed paths:
M /trunk/gcc/cse.c
M /trunk/gcc/libgcc2.c
M /trunk/gcc/reload.c
M /trunk/gcc/reload1.c
*** empty log message ***
------------------------------------------------------------------------
Again, a check-in without comment, that fixes at least 3 different
bugs, without test case, and of course the gcc-patches archives do only
go back to 1998...
Different era. While I could go back and look at the archives for the
private development list that existed at that time, it's highly likely
there was no patch posted or discussed -- and there wasn't any kind of a
regression testsuite at the time either (c-torture as a separate package
from Tege showed up in 1993 IIRC).
Jeff
