[Bug target/65342] [5 Regression] FAIL: gfortran.dg/intrinsic_(un)?pack_1.f90 -O1 execution test on powerpc-apple-darwin9 after r210201

[dominiq at lps dot ens.fr]

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

--- Comment #16 from Dominique d'Humieres dominiq at lps dot ens.fr ---
 Note that the patch in comment 6 also fixes this PR. It is probably the best
 short term solution. Could it be committed for 5.1?

Results with this patch at
https://gcc.gnu.org/ml/gcc-testresults/2015-04/msg00483.html. The failures are
also present without the patch.

[Bug target/65342] [5 Regression] FAIL: gfortran.dg/intrinsic_(un)?pack_1.f90 -O1 execution test on powerpc-apple-darwin9 after r210201

[iains at gcc dot gnu.org]

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

--- Comment #15 from Iain Sandoe iains at gcc dot gnu.org ---
(In reply to Dominique d'Humieres from comment #14)
 Results with the patch in comment 11 at
 
 https://gcc.gnu.org/ml/gcc-testresults/2015-03/msg02484.html
 
 Note that the patch in comment 6 also fixes this PR. It is probably the best
 short term solution. Could it be committed for 5.1?

I concur, there's no way I'm going to get any more cycles on this before GCC5
RC1 is built (probably in the next day or two).

Will try to find some time to address it properly (since there is a real
underlying issue) for 6/5.2.

[Bug target/65342] [5 Regression] FAIL: gfortran.dg/intrinsic_(un)?pack_1.f90 -O1 execution test on powerpc-apple-darwin9 after r210201

[dominiq at lps dot ens.fr]

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

--- Comment #14 from Dominique d'Humieres dominiq at lps dot ens.fr ---
Results with the patch in comment 11 at

https://gcc.gnu.org/ml/gcc-testresults/2015-03/msg02484.html

Note that the patch in comment 6 also fixes this PR. It is probably the best
short term solution. Could it be committed for 5.1?

[Bug target/65342] [5 Regression] FAIL: gfortran.dg/intrinsic_(un)?pack_1.f90 -O1 execution test on powerpc-apple-darwin9 after r210201

[iains at gcc dot gnu.org]

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

--- Comment #11 from Iain Sandoe iains at gcc dot gnu.org ---
Created attachment 35039
  -- https://gcc.gnu.org/bugzilla/attachment.cgi?id=35039action=edit
Patch for discussion

OK so this is a frustrating area to debug.  One can see the problem easily
enough - but finding where it's introduced ... 

Question:
 Is there a reason that we don't have a constraint for DS-mode operands?
 Since there are so few constraint letters left, I have not attempted to add
one - but curious about the reason.

The patch is intended to do the following:
 1. record that macho-pic indirections are always sufficiently aligned that a
ds-mode offset is applicable (they reside in special sections with guaranteed
alignment) - the picbase is also guaranteed to be aligned to 32b since it's a
code label.
 - changes to config/darwin.{c,h}

 2. I've added a new predicate (put it in darwin.md, for now since I'm just
testing).
 - the predicate tries to look through the various cases to determine when a
ds-mode operand is safe.

 3. This is the bit that is in generic code and needs review:

ISTM that the Y constraint code needs an additional check - at least for Darwin
- if *only* for darwin, then I'm happy to conditionalise it on TARGET_MACHO. 
However, it's not obvious to me that no other target could be affected.

What can happen is that the offset to the mem returns NULL_RTX (no offset) but
there's actually quite a complex address expression - and that might not
resolve to a sufficiently aligned object.  Presently, the code just assumes
that no offset means it's OK.


--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -6378,11 +6378,20 @@ mem_operand_gpr (rtx op, machine_mode mode)
 {
   unsigned HOST_WIDE_INT offset;
   int extra;
+  unsigned align = MEM_ALIGN (op);
   rtx addr = XEXP (op, 0);
-
   op = address_offset (addr);
   if (op == NULL_RTX)
-return true;
+{
+  /* No offset:
+ A naked reg is OK - any alignment works.  */
+  if (REG_P (addr))
+return true ;
+  /* Otherwise, we assume there will be a relocation, and we can't
+guarantee it will fit unless the object referred to is sufficiently
+aligned.  */
+  return align = 32;
+}


NOTE: there are some debug changes in the patch - this is not intended for
submission until point 3 is reviewed.

[Bug target/65342] [5 Regression] FAIL: gfortran.dg/intrinsic_(un)?pack_1.f90 -O1 execution test on powerpc-apple-darwin9 after r210201

[iains at gcc dot gnu.org]

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

--- Comment #13 from Iain Sandoe iains at gcc dot gnu.org ---
(In reply to Alan Modra from comment #12)
 We won't want that mem_operand_gpr change for Linux or AIX as we do the
 alignment checking of more complex expressions in legitimate_address_p.
 Do take heed to the comment about accepting odd rtl generated by reload..

Hmm.. OK.
FWIW, I did try to track through legitimate_address_p to see if i could
identify where the problem was being introduced (without much luck). Will have
to look again.

[Bug target/65342] [5 Regression] FAIL: gfortran.dg/intrinsic_(un)?pack_1.f90 -O1 execution test on powerpc-apple-darwin9 after r210201

[amodra at gmail dot com]

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

--- Comment #12 from Alan Modra amodra at gmail dot com ---
We won't want that mem_operand_gpr change for Linux or AIX as we do the
alignment checking of more complex expressions in legitimate_address_p.
Do take heed to the comment about accepting odd rtl generated by reload..

[Bug target/65342] [5 Regression] FAIL: gfortran.dg/intrinsic_(un)?pack_1.f90 -O1 execution test on powerpc-apple-darwin9 after r210201

[iains at gcc dot gnu.org]

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

--- Comment #7 from Iain Sandoe iains at gcc dot gnu.org ---
(In reply to Alan Modra from comment #6)
 Created attachment 35001 [details]
 workaround
 
 You might like to consider this patch that effectively reverts r210201 for
 Darwin.  This should cure the regression.

Yeah. I did something similar as a fall-back .. but (unless we run out of time)
I'd rather fix the underlying problem - since it occurs in other places too.

[if we do run out of time, then I'm fine with applying the fallback].

[Bug target/65342] [5 Regression] FAIL: gfortran.dg/intrinsic_(un)?pack_1.f90 -O1 execution test on powerpc-apple-darwin9 after r210201

[amodra at gmail dot com]

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

--- Comment #10 from Alan Modra amodra at gmail dot com ---
 permitted? (i.e. modifying %1, which is an input operand)

Yes.  You're outputting assembly, practically anything goes.

[Bug target/65342] [5 Regression] FAIL: gfortran.dg/intrinsic_(un)?pack_1.f90 -O1 execution test on powerpc-apple-darwin9 after r210201

[amodra at gmail dot com]

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

--- Comment #6 from Alan Modra amodra at gmail dot com ---
Created attachment 35001
  -- https://gcc.gnu.org/bugzilla/attachment.cgi?id=35001action=edit
workaround

You might like to consider this patch that effectively reverts r210201 for
Darwin.  This should cure the regression.

[Bug target/65342] [5 Regression] FAIL: gfortran.dg/intrinsic_(un)?pack_1.f90 -O1 execution test on powerpc-apple-darwin9 after r210201

[amodra at gmail dot com]

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

--- Comment #9 from Alan Modra amodra at gmail dot com ---
As far as fixing the real underlying problem goes, I'm not so familiar with the
darwin support that I can state with certainty that you need to fix movdi_low
and friends.

It might help to explain why powerpc64-linux -mcmodel=medium code doesn't hit
the intrinsic_unpack problem even though the resultant code looks very similar
to darwin code.  We don't see a similar propagation of offset into memory load
because legitimate_addres_p() says that isn't valid.  It isn't valid because
legitimate_constant_pool_address_p (also handles toc-relative medium model
addresses) checks alignment.  So fwprop and combine don't create the problem
memory loads in the first place.  However, if we did see such a memory load,
reload would fix it for us due to the predicate and constraints in
movdi_internal64.  Note that having legitimate_address_p() prevent these
addresses does mean we lose some optimization opportunites, but on the other
hand, if we allowed everything early we'd end up with lots of reloads and
probably worse code.

You may be able to do something similar in legitimate_address_p for darwin.

[Bug target/65342] [5 Regression] FAIL: gfortran.dg/intrinsic_(un)?pack_1.f90 -O1 execution test on powerpc-apple-darwin9 after r210201

[iains at gcc dot gnu.org]

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

--- Comment #8 from Iain Sandoe iains at gcc dot gnu.org ---
BTW, is:

(define_insn movdi_low_st
  [(set (mem:DI (lo_sum:DI (match_operand:DI 1 gpc_reg_operand b,b,b)
   (match_operand 2  Y,,)))
(match_operand:DI 0 gpc_reg_operand r,r,*!d))]
  TARGET_MACHO  TARGET_64BIT
  *
{
  switch (which_alternative)
{
  case 0:
return \std %0,lo16(%2)(%1)\;
  case 1:
{
  output_asm_insn (\la %1,lo16(%2)(%1)\, operands);

^
permitted? (i.e. modifying %1, which is an input operand)
  return (\std %0,0(%1)\);
}
  case 2:
return \stfd %0,lo16(%2)(%1)\;
  default:
gcc_unreachable ();
}
}
  [(set_attr type store)
   (set_attr length 4)])

[Bug target/65342] [5 Regression] FAIL: gfortran.dg/intrinsic_(un)?pack_1.f90 -O1 execution test on powerpc-apple-darwin9 after r210201

[iains at gcc dot gnu.org]

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

--- Comment #5 from Iain Sandoe iains at gcc dot gnu.org ---
(In reply to Alan Modra from comment #4)
 Here's another failing powerpc-darwin testcase due to movdi_low
 (movdf_low_di and their store counterparts have the same problem of course).
 
 /* -m64 -O1 -S -fno-pic  */

for Darwin, -mdynamic-no-pic ^ would be a real-life User case.

 struct {
   char c;
   long l;
 } __attribute__ ((__packed__)) x;
 
 long get_x (void)
 {
   return x.l;
 }

Thanks, Alan.
I took a quick look over the weekend (out of the office last week).

So, do you think I need to split things up so that the d-mode and ds-mode insn
fragments can have different match conditions? - or is the issue that these
patterns have no reload constraints (in which case I could at least investigate
adding Y to the relevant cases)?

[Bug target/65342] [5 Regression] FAIL: gfortran.dg/intrinsic_(un)?pack_1.f90 -O1 execution test on powerpc-apple-darwin9 after r210201

[amodra at gmail dot com]

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

Alan Modra amodra at gmail dot com changed:

   What|Removed |Added

 CC|amodra at gcc dot gnu.org  |

--- Comment #4 from Alan Modra amodra at gmail dot com ---
Here's another failing powerpc-darwin testcase due to movdi_low (movdf_low_di
and their store counterparts have the same problem of course).

/* -m64 -O1 -S -fno-pic  */
struct {
  char c;
  long l;
} __attribute__ ((__packed__)) x;

long get_x (void)
{
  return x.l;
}

[Bug target/65342] [5 Regression] FAIL: gfortran.dg/intrinsic_(un)?pack_1.f90 -O1 execution test on powerpc-apple-darwin9 after r210201

[rguenth at gcc dot gnu.org]

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

Richard Biener rguenth at gcc dot gnu.org changed:

   What|Removed |Added

   Target Milestone|--- |5.0

[Bug target/65342] [5 Regression] FAIL: gfortran.dg/intrinsic_(un)?pack_1.f90 -O1 execution test on powerpc-apple-darwin9 after r210201

[jakub at gcc dot gnu.org]

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

Jakub Jelinek jakub at gcc dot gnu.org changed:

   What|Removed |Added

   Priority|P3  |P4
 CC||jakub at gcc dot gnu.org

[Bug target/65342] [5 Regression] FAIL: gfortran.dg/intrinsic_(un)?pack_1.f90 -O1 execution test on powerpc-apple-darwin9 after r210201

[iains at gcc dot gnu.org]

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

--- Comment #3 from Iain Sandoe iains at gcc dot gnu.org ---
(In reply to Dominique d'Humieres from comment #2)
  Confirmed.  The problem occurs in fwprop1 where instructions corresponding 
  to the
  following assembly
  addis r2,r31,ha16(_A.1.1600-L1$pb)
  la r9,lo16(_A.1.1600-L1$pb)(r2)
  ld r2,0(r9)
  are combined to
  addis r2,r31,ha16(_A.1.1600-L1$pb)
  la r9,lo16(_A.1.1600-L1$pb)(r2)
  ld r2,lo16(_A.1.1600-L1$pb)(r2)
  ie. the offset is propagated into the memory load.  This ought to give you
  an error at assembly or link time.
 
 No error at assembly or link time.

  If not, you have a bad assembler or linker..
 
 Well, we'll have to live with them!-(EOL target).

The system as is based on gas-1.38 and doesn't warn for this :-(
ld64 is slightly better and does catch a few more cases (where they resolve at
link-time).

.. there's some hope for my WIP GAS port and an updated ld64 (yeah, I know it's
taking a long time for these to appear) ..

  movdi_low is the culprit, I think.  It should require a suitably aligned
  offset (operand 2).
 
 How?

In the mdynamic-no-pic case, the literal value should work the same as for
linux.

In the case of PIC, I suspect we need to look through the uspecs that wrap
mach-o PIC offsets and try to determine if the alignment of the referenced
symbol is guaranteed to be enough.  The alignment of the picbase will always
be = 4.

Some time ago I had a WIP patch to try and deal with this … but it's bit-rotted
so needs a significant re-visit.

[Bug target/65342] [5 Regression] FAIL: gfortran.dg/intrinsic_(un)?pack_1.f90 -O1 execution test on powerpc-apple-darwin9 after r210201

[dominiq at lps dot ens.fr]

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

--- Comment #2 from Dominique d'Humieres dominiq at lps dot ens.fr ---
 Confirmed.  The problem occurs in fwprop1 where instructions corresponding to 
 the
 following assembly
   addis r2,r31,ha16(_A.1.1600-L1$pb)
   la r9,lo16(_A.1.1600-L1$pb)(r2)
   ld r2,0(r9)
 are combined to
   addis r2,r31,ha16(_A.1.1600-L1$pb)
   la r9,lo16(_A.1.1600-L1$pb)(r2)
   ld r2,lo16(_A.1.1600-L1$pb)(r2)
 ie. the offset is propagated into the memory load.  This ought to give you
 an error at assembly or link time.

No error at assembly or link time.

 If not, you have a bad assembler or linker..

Well, we'll have to live with them!-(EOL target).

 movdi_low is the culprit, I think.  It should require a suitably aligned
 offset (operand 2).

How?

[Bug target/65342] [5 Regression] FAIL: gfortran.dg/intrinsic_(un)?pack_1.f90 -O1 execution test on powerpc-apple-darwin9 after r210201

[amodra at gmail dot com]

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

Alan Modra amodra at gmail dot com changed:

   What|Removed |Added

 Status|UNCONFIRMED |NEW
   Last reconfirmed||2015-03-07
 CC||amodra at gmail dot com
 Ever confirmed|0   |1

--- Comment #1 from Alan Modra amodra at gmail dot com ---
Confirmed.  The problem occurs in fwprop1 where instructions corresponding to
the following assembly
addis r2,r31,ha16(_A.1.1600-L1$pb)
la r9,lo16(_A.1.1600-L1$pb)(r2)
ld r2,0(r9)
are combined to
addis r2,r31,ha16(_A.1.1600-L1$pb)
la r9,lo16(_A.1.1600-L1$pb)(r2)
ld r2,lo16(_A.1.1600-L1$pb)(r2)
ie. the offset is propagated into the memory load.  This ought to give you an
error at assembly or link time.  If not, you have a bad assembler or linker..

movdi_low is the culprit, I think.  It should require a suitably aligned offset
(operand 2).