[Bug c/63783] New: gcc-4.9: Miscompilation of boolean negation on SH4 using -O2

[Bug c/63783] New: gcc-4.9: Miscompilation of boolean negation on SH4 using -O2

[glaubitz at physik dot fu-berlin.de]

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

            Bug ID: 63783
           Summary: gcc-4.9: Miscompilation of boolean negation on SH4
                    using -O2
           Product: gcc
           Version: 4.9.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: glaubitz at physik dot fu-berlin.de

Hello!

Recently we ran into linker issues on our SH4 buildds in Debian [1]. We
initially blamed the linker for the problem, but after some more investigation,
Michael Karcher was eventually able to track down the problem being a
regression in gcc [2].

When compiling the following code snippet with optimization -O2 enabled, the
assertation will fail but succeed without:

#include <assert.h>

int decision_result;
int truecount = 0;
int val;

void buggy(int flag)
{
  int condition;
  if(flag == 0)
    condition = val != 0;
  else
    condition = !decision_result;
  if (condition)
  {
     truecount++;
  }
}

int main(void)
{
  decision_result = 1;
  buggy(1);
  assert(truecount == 0);
}

The issue did not arise with gcc-4.6, for example, so it appears to be a
regression with one of the later versions of gcc.

Cheers,
Adrian

> [1] https://sourceware.org/bugzilla/show_bug.cgi?id=17553
> [2] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=768574

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[kkojima at gcc dot gnu.org]

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

Kazumoto Kojima <kkojima at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|                            |sh*-*-*
             Status|UNCONFIRMED                 |NEW
      Known to work|                            |4.8.3
           Keywords|                            |wrong-code
   Last reconfirmed|                            |2014-11-09
          Component|c                           |target
                 CC|                            |kkojima at gcc dot gnu.org,
                   |                            |olegendo at gcc dot gnu.org
     Ever confirmed|0                           |1
            Summary|gcc-4.9: Miscompilation of  |[4.9/5 Regression] [SH]
                   |boolean negation on SH4     |Miscompilation of boolean
                   |using -O2                   |negation on SH4 using -O2
      Known to fail|                            |4.9.0

--- Comment #1 from Kazumoto Kojima <kkojima at gcc dot gnu.org> ---
I'd like to add oleg to the CC list.
This is a 4.9/5.0 regression from 4.8.  4.8 compilers generate
the code like

    tst    r1,r1
    movt    r1

for condition = !decision_result and we get

    not    r1,r1

for it with 4.9 and the newer compilers.
It seems that sh_treg_combine phase added at 4.9 does this.

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[olegendo at gcc dot gnu.org]

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

--- Comment #2 from Oleg Endo <olegendo at gcc dot gnu.org> ---
Thanks for tracing this down.  Yes, it looks like a sh_treg_combine bug.  I
will have a look at it, but probably only in 2 weeks from now.

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[gcc-bugzilla at mkarcher dot dialup.fu-berlin.de]

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

Michael Karcher <gcc-bugzilla at mkarcher dot dialup.fu-berlin.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |gcc-bugzilla at mkarcher dot dialu
                   |                            |p.fu-berlin.de

--- Comment #3 from Michael Karcher <gcc-bugzilla at mkarcher dot dialup.fu-berlin.de> ---
Created attachment 33954
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=33954&action=edit
Remove assumption that "not" is logical negate

Indeed the treg combine pass is broken. Thanks to the nicely documented code,
it could easily be determined, that this optimization pass contains the wrong
assumption that "not" is an assembler instruction that can be used for logical
negation of a register. As this is a bitwise negate instruction, it can not be
used that way.

As I was unable to find a machine instruction that performs logical negation, I
prepared a patch that completely removes the parts that rely on a logical
negate instruction, while keeping all other aspects on the optimization pass
intact.

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[olegendo at gcc dot gnu.org]

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

--- Comment #4 from Oleg Endo <olegendo at gcc dot gnu.org> ---
(In reply to Michael Karcher from comment #3)
> Created attachment 33954 [details]
> Remove assumption that "not" is logical negate
> 
> Indeed the treg combine pass is broken. Thanks to the nicely documented
> code, it could easily be determined, that this optimization pass contains
> the wrong assumption that "not" is an assembler instruction that can be used
> for logical negation of a register. As this is a bitwise negate instruction,
> it can not be used that way.

I'm not sure about this.  The first hunk of your patch that removes the example
in the top comment block should be valid, as far as I can see at the moment. 
This is because the result of the bitwise not is then again tested for zero
before the conditional jump.
I haven't looked at the details (RTL dumps etc), but looking at the problematic
code in the description, the problem could be that the variable 'condition'
gets the wrong value assigned.  'condition' should have a value of either 0 or
1.  But the sh_treg_combine pass changes the value.  If the conditional jump is
eliminated and the 'condition' value is then added to 'truecount' directly, it
will produce wrong values. 

> As I was unable to find a machine instruction that performs logical
> negation, I prepared a patch that completely removes the parts that rely on
> a logical negate instruction, while keeping all other aspects on the
> optimization pass intact.

Thanks for the patch.  Before completely eliminating the bitwise not insn, I'd
like to do some further analysis and try to keep the bitwise not except for
cases, where it would change the code semantics.

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[gcc-bugzilla at mkarcher dot dialup.fu-berlin.de]

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

--- Comment #5 from Michael Karcher <gcc-bugzilla at mkarcher dot dialup.fu-berlin.de> ---
(In reply to Oleg Endo from comment #4)
> I'm not sure about this.  The first hunk of your patch that removes the
> example in the top comment block should be valid, as far as I can see at the
> moment.  This is because the result of the bitwise not is then again tested
> for zero before the conditional jump.

As far as I understand the SH4 architecture, "TST r0,r0" sets the T bit if r0
is zero, and clears it otherwise. So
  MOV   r1,r0        ; store r1 to r0, for exposition only
  TST   r0,r0        ; T bit set if r1 == 0, cleared if r1 != 0
  MOVT  r0           ; r0 set to 1 if r1 == 0, cleared if r1 != 0
  TST   r0,r0        ; T bit set if r1 != 0, cleared if r1 == 0
while on the other hand
  NOT   r1,r0        ; bitwise negate r1 to r0 (so r0 gets 0 if r1 == -1)
  TST   r0,r0        ; T bit set if r1 == -1, cleared if r1 != -1

The first block clear T only if r1 equals zero, the second block clears T for
all values of r1 except -1. So they are only equivalent if you can prove that
r1 is either zero or -1, which you typically can't, especially as using 1 for
true is typical C semantic. Because it seems that it is rare you can
successfully prove that r1 either 0 or -1, I decided to remove that
transformation alltogether. And to keep the comment accurate, I had to remove
it from there, too.

For the transformation to be valid, you would need a logical not instruction
instead of the bitwise not instruction that sets the desination register to
zero if the source register contains any non-zero value, not just if the source
register contains -1.

Possibly your confusion is created by "NOTT" (if it were available on SH4)
being in fact valid in that example to negate the condition. This is due to the
fact, that for 1-bit values (only!) bitwise negation and boolean negation are
the same.

> I haven't looked at the details (RTL dumps etc), but looking at the
> problematic code in the description, the problem could be that the variable
> 'condition' gets the wrong value assigned.  'condition' should have a value
> of either 0 or 1.  But the sh_treg_combine pass changes the value.  If the
> conditional jump is eliminated and the 'condition' value is then added to
> 'truecount' directly, it will produce wrong values. 

Looking at the generated assembly code, I verified that indeed the pattern
above is used, so "condition" gets stored in the "T" bit, and a conditional
branch (BF) is used to skip the incrementation if it is false, i.e. cleared.
After applying the transformation shown above, the value 1 in decision_result
gets negated to -2 (0xFFFFFFFE), which is non-zero, so truecount gets increased
if even though decision_result should be false after boolean negation.

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[olegendo at gcc dot gnu.org]

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

--- Comment #6 from Oleg Endo <olegendo at gcc dot gnu.org> ---
(In reply to Michael Karcher from comment #5)
> (In reply to Oleg Endo from comment #4)
> > I'm not sure about this.  The first hunk of your patch that removes the
> > example in the top comment block should be valid, as far as I can see at the
> > moment.  This is because the result of the bitwise not is then again tested
> > for zero before the conditional jump.

... the above is true for bitwise not values of zero only...

> As far as I understand the SH4 architecture, "TST r0,r0" sets the T bit if
> r0 is zero, and clears it otherwise. So
>   MOV   r1,r0        ; store r1 to r0, for exposition only
>   TST   r0,r0        ; T bit set if r1 == 0, cleared if r1 != 0
>   MOVT  r0           ; r0 set to 1 if r1 == 0, cleared if r1 != 0
>   TST   r0,r0        ; T bit set if r1 != 0, cleared if r1 == 0
> while on the other hand
>   NOT   r1,r0        ; bitwise negate r1 to r0 (so r0 gets 0 if r1 == -1)
>   TST   r0,r0        ; T bit set if r1 == -1, cleared if r1 != -1
> 
> The first block clear T only if r1 equals zero, the second block clears T
> for all values of r1 except -1. So they are only equivalent if you can prove
> that r1 is either zero or -1, which you typically can't, especially as using
> 1 for true is typical C semantic. Because it seems that it is rare you can
> successfully prove that r1 either 0 or -1, I decided to remove that
> transformation alltogether. And to keep the comment accurate, I had to
> remove it from there, too.

... and because of this, you're right that the bitwise not can't be used for
this kind of thing.  That was my thinko when writing the pass.  If the input
value for the bitwise not is something != -1 then it will not output zero
obviously and this is wrong.

> For the transformation to be valid, you would need a logical not instruction
> instead of the bitwise not instruction that sets the desination register to
> zero if the source register contains any non-zero value, not just if the
> source register contains -1.

There is a 2 insn sequence to do that on SH:
   tst  rm,rm
   movt rn

As far as I can see it, emitting this sequence instead of the bitwise not insn
would fix the problem.  This can be done by changing the function
sh_treg_combine::make_not_reg_insn.  It should be safe to emit that sequence
(which clobbers the T reg) instead of the bitwise not.

> Possibly your confusion is created by "NOTT" (if it were available on SH4)
> being in fact valid in that example to negate the condition. This is due to
> the fact, that for 1-bit values (only!) bitwise negation and boolean
> negation are the same.

No, this confusion is not related to the SH2A nott insn.  I was just thinking
of input values of zero for the bitwise not.

> Looking at the generated assembly code, I verified that indeed the pattern
> above is used, so "condition" gets stored in the "T" bit, and a conditional
> branch (BF) is used to skip the incrementation if it is false, i.e. cleared.
> After applying the transformation shown above, the value 1 in
> decision_result gets negated to -2 (0xFFFFFFFE), which is non-zero, so
> truecount gets increased if even though decision_result should be false
> after boolean negation.

OK, thanks for the confirmation.  I had a closer look at the current code and
your patch.  I think that the bitwise-not code paths should not be removed
entirely, as they handle the "mixed normal and inverting cstores" cases.  I
will prepare a patch in a couple of days.

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[gcc-bugzilla at mkarcher dot dialup.fu-berlin.de]

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

--- Comment #7 from Michael Karcher <gcc-bugzilla at mkarcher dot dialup.fu-berlin.de> ---
(In reply to Oleg Endo from comment #6)
> > For the transformation to be valid, you would need a logical not instruction
> > instead of the bitwise not instruction that sets the desination register to
> > zero if the source register contains any non-zero value, not just if the
> > source register contains -1.
> 
> There is a 2 insn sequence to do that on SH:
>    tst  rm,rm
>    movt rn
> 
> As far as I can see it, emitting this sequence instead of the bitwise not
> insn would fix the problem.  This can be done by changing the function
> sh_treg_combine::make_not_reg_insn.  It should be safe to emit that sequence
> (which clobbers the T reg) instead of the bitwise not.

This seems definitely true. But I wonder whether this is acutally needed, or
already caught perfectly in try_eliminate_cstores. Let's have a look:

The buggy branch in try_combine_comparisons applies only if (first loop)
 - All comparisons are the same type
 - The second operands are either all the same or all registers
 - Removal of those instruction is safe
Furthermore, there need to be mixed cstores and
 - All minority comparisons need to be comparisons
 - All minority comparisons need to be against the integral constant zero

So if one combines what applies to minority stores only in the test for
applicability of the "make_not_reg_insn" idea with the general applicability
check of the try_combine_comparisons pass, the end result is:

The buggy branch only applies if
 - All comparisons are equality comparisons
 - The second operand needs to be the integral constant zero.

Now this is exactly what you correctly commented in "example 1":

 [bb 3]
 (set (reg:SI 147 t) (eq:SI (reg:SI 173) (const_int 0)))
 (set (reg:SI 167) (xor:SI (reg:SI 147 t) (const_int 1)))
 -> bb 5

 [bb 4]
 (set (reg:SI 147 t) (eq:SI (reg:SI 177) (const_int 0)))
 (set (reg:SI 167) (reg:SI 147 t))
 -> bb 5

 [bb 5]
 (set (reg:SI 147 t) (eq:SI (reg:SI 167) (const_int 0)))
 (set (pc) (if_then_else (ne (reg:SI 147 t) (const_int 0))
                        (label_ref:SI 50) (pc)))

You are going to transform this to, assuming bb 4 is considered minority. (The
first RTL instruction in BB4 gets transformed to "test rm, rm", the second RTL
instruction gets transformed to "movt rn".)

 [bb 3]
 (set (reg:SI 167) (reg:SI 173))
 -> bb 5

 [bb 4]
 (set (reg:SI 147 t) (eq:SI (reg:SI 173) (const_int 0)))
 (set (reg:SI 167) (xor:SI (reg:SI 147 t) (const_int 1)))
 -> bb 5

 [bb 5]
 (set (reg:SI 147 t) (eq:SI (reg:SI 167) (const_int 0)))
 (set (pc) (if_then_else (ne (reg:SI 147 t) (const_int 0)))
                         (label_ref:SI 50) (pc)))

It is afterwards quite likely that the set instruction in bb3 can be elminated
by renaming reg 167 to reg 173.

If this transformation is removed, though, the transformation of example 4
applies instead, which will result in

 [bb 3]
 (set (reg:SI 147 t) (eq:SI (reg:SI 173) (const_int 0)))
 -> bb 5

 [bb 4]
 (set (reg:SI 147 t) (eq:SI (reg:SI 176) (const_int 0)))
 (set (reg:SI 147 t) (xor:SI (reg:SI 147 t) (const_int 1)))
 -> bb 5

 [bb 5]
 (set (pc) (if_then_else (eq (reg:SI 147 t) (const_int 0))  // inverted
                         (label_ref:SI 50) (pc)))

and this gets (except SH2A with nott) transformed to (by define_insn_and_split
"nott" in the machine definition)

 [bb 3]
 (set (reg:SI 147 t) (eq:SI (reg:SI 173) (const_int 0)))
 -> bb 5

 [bb 4]
 (set (reg:SI 147 t) (eq:SI (reg:SI 176) (const_int 0)))
 (set (reg:SI 200) (xor:SI (reg:SI 147 t) (const_int 1)))
 (set (reg:SI 147 t) (eq:SI (reg:SI 200) (const_int 0)))
 -> bb 5

 [bb 5]
 (set (pc) (if_then_else (eq (reg:SI 147 t) (const_int 0))  // inverted
                         (label_ref:SI 50) (pc)))

which, at least in my example, seems to be transformed by register renaming and
common code elimination (combining the to treg setting instructions at the end
of bb3 and bb4) to something like

 [bb 3]
 -> bb 5

 [bb 4]
 (set (reg:SI 147 t) (eq:SI (reg:SI 176) (const_int 0)))
 (set (reg:SI 173) (xor:SI (reg:SI 147 t) (const_int 1)))
 -> bb 5

 [bb 5]
 (set (reg:SI 147 t) (eq:SI (reg:SI 173) (const_int 0)))
 (set (pc) (if_then_else (eq (reg:SI 147 t) (const_int 0))  // inverted
                         (label_ref:SI 50) (pc)))

which is exactly the result you also get with special-casing the
compare-to-zero case.

> OK, thanks for the confirmation.  I had a closer look at the current code
> and your patch.  I think that the bitwise-not code paths should not be
> removed entirely, as they handle the "mixed normal and inverting cstores"
> cases.  I will prepare a patch in a couple of days.

If I am not mistaken in my analysis above, the "mixed cstores" case handling in
try_eliminate_cstores does already what you need, while pushing the desicision
whether those are "comparisons to zero" and can be further optimized on common
subexpression elimination, instead of hand-written code checking for a special
"compare to zero" pattern.

Disclaimer: I performed no further test than looking at machine code generated
for the example program given in the bug report.

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[gcc-bugzilla at mkarcher dot dialup.fu-berlin.de]

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

--- Comment #8 from Michael Karcher <gcc-bugzilla at mkarcher dot dialup.fu-berlin.de> ---
Actually, the whole issue got me curious - I will try prepare a different patch
along your suggestions and compare the compiler output. If I don't report back
today, I probably won't do that in time, so don't hold your breath, though.

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[olegendo at gcc dot gnu.org]

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

--- Comment #9 from Oleg Endo <olegendo at gcc dot gnu.org> ---
(In reply to Michael Karcher from comment #8)
> Actually, the whole issue got me curious - I will try prepare a different
> patch along your suggestions and compare the compiler output. If I don't
> report back today, I probably won't do that in time, so don't hold your
> breath, though.

Sure, improvements are always welcome.  Unfortunately, target independent CSE
and other RTL passes didn't work so well on SH in this case, that's why I've
added the custom RTL pass.  For the history of this issue please see PR 51244.

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[gcc-bugzilla at mkarcher dot dialup.fu-berlin.de]

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

--- Comment #11 from Michael Karcher <gcc-bugzilla at mkarcher dot dialup.fu-berlin.de> ---
Putting things straight after trying it out:

(In reply to Michael Karcher from comment #7)
[...]
> and this gets (except SH2A with nott) transformed to (by
> define_insn_and_split "nott" in the machine definition)
> 
>  [bb 3]
>  (set (reg:SI 147 t) (eq:SI (reg:SI 173) (const_int 0)))
>  -> bb 5
> 
>  [bb 4]
>  (set (reg:SI 147 t) (eq:SI (reg:SI 176) (const_int 0)))
>  (set (reg:SI 200) (xor:SI (reg:SI 147 t) (const_int 1)))
This is WRONG. It should read "(set (reg:SI 200) (reg:SI 147 t))" instead!

>  (set (reg:SI 147 t) (eq:SI (reg:SI 200) (const_int 0)))
>  -> bb 5
> 
>  [bb 5]
>  (set (pc) (if_then_else (eq (reg:SI 147 t) (const_int 0))  // inverted
>                          (label_ref:SI 50) (pc)))
> 
> which, at least in my example, seems to be transformed by register renaming
> and common code elimination (combining the to treg setting instructions at
> the end of bb3 and bb4) to something like
This turns out to be wrong too. The treg setting instructions are NOT combined.
See the previous comment.

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[gcc-bugzilla at mkarcher dot dialup.fu-berlin.de]

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

--- Comment #12 from Michael Karcher <gcc-bugzilla at mkarcher dot dialup.fu-berlin.de> ---
Further digging into this showed that there actually is a pass that would merge
the two "tst r1,r1" instructions - the jump2 pass in cfgclenup.c.

The optimization is called "crossjumping" in gcc, also known as tail merging.
For some reason[1], gcc is reluctant to perform crossjumping on small common
parts. In this case, the common part is just one instruction, and the parameter
min-crossjump-insns is five by default unless optimizing for size. With
"-fparam-min-crossjump-insns=1" or "-Os", my first patch removing all the
special-casing of zero compares produces the same result as the patch fixing
the logical negations. In the current situation, I see no advantage of not
cross-jumping in this case, so the minimally invasive solution is definitely
the second patch to fix logical negation, but still it somehow feels ugly to me
to have a limited reimplementation of crossjumping in the sh-treg-combine pass.
Replacing stuff of that pass by improving other optimizations is IMHO beyond
the scope of this bug, though.

[1] These are actually performance reasons:
https://gcc.gnu.org/ml/gcc-patches/2004-07/msg00495.html

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[olegendo at gcc dot gnu.org]

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

--- Comment #13 from Oleg Endo <olegendo at gcc dot gnu.org> ---
(In reply to Michael Karcher from comment #12)
> Further digging into this showed that there actually is a pass that would
> merge the two "tst r1,r1" instructions - the jump2 pass in cfgclenup.c.
> 
> The optimization is called "crossjumping" in gcc, also known as tail
> merging. For some reason[1], gcc is reluctant to perform crossjumping on
> small common parts. In this case, the common part is just one instruction,
> and the parameter min-crossjump-insns is five by default unless optimizing
> for size. With "-fparam-min-crossjump-insns=1" or "-Os", my first patch
> removing all the special-casing of zero compares produces the same result as
> the patch fixing the logical negations. In the current situation, I see no
> advantage of not cross-jumping in this case, so the minimally invasive
> solution is definitely the second patch to fix logical negation, but still
> it somehow feels ugly to me to have a limited reimplementation of
> crossjumping in the sh-treg-combine pass. Replacing stuff of that pass by
> improving other optimizations is IMHO beyond the scope of this bug, though.

Thanks for digging into that.

One possible way to evaluate the crossjumping optimization in ths case:
- disable/don't register sh_treg_combine pass in sh.c
- set default param -fparam-min-crossjump-insns=1 somehow
- run sh.exp tests
- compare CSiBE code sizes (or similar larger code bases)

If you're interested in doing that, please post your findings in PR 51244.

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.9.3

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[olegendo at gcc dot gnu.org]

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

--- Comment #14 from Oleg Endo <olegendo at gcc dot gnu.org> ---
Created attachment 34062
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=34062&action=edit
Proposed patch

I'm now testing the attached patch.

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[gcc-bugzilla at mkarcher dot dialup.fu-berlin.de]

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

--- Comment #15 from Michael Karcher <gcc-bugzilla at mkarcher dot dialup.fu-berlin.de> ---
I did not get around to test your proposed patch yet, but it seems like the new
"logical not" operation always compares only the low 32 bit against zero, even
if there is a 64 bit operand. If my analysis is correct, the long long test
program should fail if you replace "decision = 1;" by "decision =
0x100000000LL;"

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[olegendo at gcc dot gnu.org]

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

Oleg Endo <olegendo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #34062|0                           |1
        is obsolete|                            |

--- Comment #16 from Oleg Endo <olegendo at gcc dot gnu.org> ---
Created attachment 34072
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=34072&action=edit
Proposed patch

Other existing test cases needed some adjustments.  Tested on trunk with
make -k check RUNTESTFLAGS="--target_board=sh-sim\{-m4/-ml,-m4/-mb}"

and no new failures.  Applies also on 4.9 branch without problems.  Tested on
4.9 briefly with make all-gcc and checking the generated code for the
problematic test case.  I will apply this patch if there are no further
objections.

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[olegendo at gcc dot gnu.org]

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

--- Comment #17 from Oleg Endo <olegendo at gcc dot gnu.org> ---
(In reply to Michael Karcher from comment #15)
> I did not get around to test your proposed patch yet, but it seems like the
> new "logical not" operation always compares only the low 32 bit against
> zero, even if there is a 64 bit operand. If my analysis is correct, the long
> long test program should fail if you replace "decision = 1;" by "decision =
> 0x100000000LL;"

I've tried that and the test still passes.  Maybe you got confused by the
following generated code for DImode test against zero:

        mov.l   @r2,r3
        mov.l   @(4,r2),r1
        or      r3,r1
        tst     r1,r1
        bra     .L3
        movt    r1

The or-tst sequence is generated quite early and the sh_treg_combine pass
actually never gets DImode comparisons fed in this case.

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[gcc-bugzilla at mkarcher dot dialup.fu-berlin.de]

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

--- Comment #18 from Michael Karcher <gcc-bugzilla at mkarcher dot dialup.fu-berlin.de> ---
As I said, I did not try your patch, but just read the source. The assembly you
quoted convinces me that there is no problem in the code actually produced by
your patch, which is great. This is caused by the pattern
or-then-SImode-compare, as you explained.

The or-then-SImode-compare optimization has an adverse effect on the test
coverage, it seems. In both cases, GET_MODE(src_reg) and GET_MODE(dst_reg) are
SImode, so the DImode output branch is not tested by any of your two example
source files. Furthermore, it looks like make_not_reg_insn will actually
produce bad code if it were ever called with GET_MODE(src_reg) == DImode. So I
would strongly suggest to narrow it to only accept SImode input operands, so it
fails to apply instead of generate bad code if something manages to call it
with DImode input.

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[olegendo at gcc dot gnu.org]

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

--- Comment #19 from Oleg Endo <olegendo at gcc dot gnu.org> ---
(In reply to Michael Karcher from comment #18)
> As I said, I did not try your patch, but just read the source. The assembly
> you quoted convinces me that there is no problem in the code actually
> produced by your patch, which is great. This is caused by the pattern
> or-then-SImode-compare, as you explained.
> 
> The or-then-SImode-compare optimization has an adverse effect on the test
> coverage, it seems. In both cases, GET_MODE(src_reg) and GET_MODE(dst_reg)
> are SImode, so the DImode output branch is not tested by any of your two
> example source files.

That is true as it stands now.  However, we already anticipate that there might
be something going on with DImode stuff, so just adding the test might help
debugging in the future.  Even if it doesn't add any value now, it doesn't hurt
anyone either.

> Furthermore, it looks like make_not_reg_insn will
> actually produce bad code if it were ever called with GET_MODE(src_reg) ==
> DImode.

Please do explain.

> So I would strongly suggest to narrow it to only accept SImode input
> operands, so it fails to apply instead of generate bad code if something
> manages to call it with DImode input.

If that should ever happen, we already have a test case that will pop up in the
test results.

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[gcc-bugzilla at mkarcher dot dialup.fu-berlin.de]

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

--- Comment #20 from Michael Karcher <gcc-bugzilla at mkarcher dot dialup.fu-berlin.de> ---
(In reply to Oleg Endo from comment #19)
> > The or-then-SImode-compare optimization has an adverse effect on the test
> > coverage, it seems. In both cases, GET_MODE(src_reg) and GET_MODE(dst_reg)
> > are SImode, so the DImode output branch is not tested by any of your two
> > example source files.
> That is true as it stands now.  However, we already anticipate that there
> might be something going on with DImode stuff, so just adding the test might
> help debugging in the future.  Even if it doesn't add any value now, it
> doesn't hurt anyone either.

The test case is not a problem - but it would be helpful to have a testcase
that actually tests the DImode output case. I understand that it likely is not
possible with today's gcc to reach that branch, so it seems this has to stay
the way it is now. I am fine with it.

> > Furthermore, it looks like make_not_reg_insn will
> > actually produce bad code if it were ever called with GET_MODE(src_reg) ==
> > DImode.
> Please do explain.

Of course. The instructions involving src_reg in make_not_reg_insn dealing with
src_reg are completely quoted here:

+  // On SH we can do only SImode and DImode comparisons.
+  if (! (GET_MODE (src_reg) == SImode || GET_MODE (src_reg) == DImode))
+    return NULL;

In this fragment, you accept DImode source operands. So that code may be used
to replace a DImode compare.

+  emit_insn (gen_rtx_SET (VOIDmode, m_ccreg,
+              gen_rtx_fmt_ee (EQ, SImode, src_reg, const0_rtx)));

In this fragment, you are generating the replacement instruction, which is
always an SImode compare. Maybe I miss the point, but I fail to undestand how
an SImode compare might be acceptable on an DImode operand. Possibly, this even
ICEs, I don't know enough about gcc internals to know what happens if src_reg
is DImode which is passed to EQ in SImode.

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[olegendo at gcc dot gnu.org]

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

--- Comment #21 from Oleg Endo <olegendo at gcc dot gnu.org> ---
(In reply to Michael Karcher from comment #20)
> Of course. The instructions involving src_reg in make_not_reg_insn dealing
> with src_reg are completely quoted here:
> 
> +  // On SH we can do only SImode and DImode comparisons.
> +  if (! (GET_MODE (src_reg) == SImode || GET_MODE (src_reg) == DImode))
> +    return NULL;
> 
> In this fragment, you accept DImode source operands. So that code may be
> used to replace a DImode compare.
> 
> +  emit_insn (gen_rtx_SET (VOIDmode, m_ccreg,
> +			  gen_rtx_fmt_ee (EQ, SImode, src_reg, const0_rtx)));
> 
> In this fragment, you are generating the replacement instruction, which is
> always an SImode compare. Maybe I miss the point, but I fail to undestand
> how an SImode compare might be acceptable on an DImode operand. Possibly,
> this even ICEs, I don't know enough about gcc internals to know what happens
> if src_reg is DImode which is passed to EQ in SImode.

The SImode refers to the result of the EQ, not the operands.  In sh.md the
pattern cmpeqdi_t will be picked up for this.
>From gcc-bugs-return-468075-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Sat Nov 22 15:35:39 2014
Return-Path: <gcc-bugs-return-468075-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 12996 invoked by alias); 22 Nov 2014 15:35:38 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 12949 invoked by uid 48); 22 Nov 2014 15:35:35 -0000
From: "iains at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug preprocessor/63831] [5 Regression] r217292 causes segfaults with -MM
Date: Sat, 22 Nov 2014 15:35:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: preprocessor
X-Bugzilla-Version: 5.0
X-Bugzilla-Keywords:
X-Bugzilla-Severity: normal
X-Bugzilla-Who: iains at gcc dot gnu.org
X-Bugzilla-Status: NEW
X-Bugzilla-Priority: P1
X-Bugzilla-Assigned-To: emsr at gcc dot gnu.org
X-Bugzilla-Target-Milestone: 5.0
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields:
Message-ID: <bug-63831-4-qXQzAioHlJ@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-63831-4@http.gcc.gnu.org/bugzilla/>
References: <bug-63831-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2014-11/txt/msg02547.txt.bz2
Content-length: 2850

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

--- Comment #13 from Iain Sandoe <iains at gcc dot gnu.org> ---
So at stage #3 building libada, we see that s-oscons.{adb,h} are empty. 
Looking at the error log :

ln: rts/system.ads: File exists
In file included from /usr/include/sys/time.h:78:0,
                 from /GCC/gcc-trunk/gcc/ada/gsocket.h:183,
                 from /GCC/gcc-trunk/gcc/ada/s-oscons-tmplt.c:103:
/usr/include/sys/_structs.h: In function ‘__has_attribute__’:
/usr/include/sys/_structs.h:186:3: error: storage class specified for parameter
‘fd_set’
 } fd_set;
   ^
/usr/include/sys/_structs.h:192:1: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or
‘__attribute__’ before ‘{’ token
 {
 ^
In file included from /GCC/gcc-trunk/gcc/ada/gsocket.h:183:0,
                 from /GCC/gcc-trunk/gcc/ada/s-oscons-tmplt.c:103:
/usr/include/sys/time.h:82:25: error: storage class specified for parameter
‘time_t’
 typedef __darwin_time_t time_t;

=== note in the stuff below:
Availability.h includes AvailabilityInternal.h which uses:
    #ifdef __has_attribute
        #if __has_attribute(availability)
            /* use better attributes if possible */

what's not obvious is why that's causing a failure in this context (but passing
elsewhere in the compile.

[of course, could be buggy system headers, but then clang works on this
platform, so I suppose it should have been exercised.]

I don't think I can share darwin headers here - but it's likely that we can
find the key ones on the apple OpenSource server, if you don't have access to
them.

In the meantime I'll see if I can narrow things down.


==== /GCC/gcc-trunk/gcc/ada/s-oscons-tmplt.c
    102 
    103 #include "gsocket.h"
    104 
==== /GCC/gcc-trunk/gcc/ada/gsocket.h
    180 #if defined (__vxworks) && ! defined (__RTP__)
    181 #include <sys/times.h>
    182 #else
    183 #include <sys/time.h>
    184 #endif
==== /usr/include/sys/time.h
     64 #ifndef _SYS_TIME_H_
     65 #define _SYS_TIME_H_
     66 
     67 #include <sys/cdefs.h>
     68 #include <sys/_types.h>
     69 #include <Availability.h>
     70 
     71 /*
     72  * [XSI] The fd_set type shall be defined as described in
<sys/select.h>.
     73  * The timespec structure shall be defined as described in <time.h>
     74  */
     75 #define __need_fd_set
     76 #define __need_struct_timespec
     77 #define __need_struct_timeval
     78 #include <sys/_structs.h>
==== /usr/include/sys/_structs.h
    183 __BEGIN_DECLS
    184 typedef struct fd_set {
    185         __int32_t       fds_bits[__DARWIN_howmany(__DARWIN_FD_SETSIZE,
__DARWIN_NFDBITS)];
    186 } fd_set;
    187 __END_DECLS
>From gcc-bugs-return-468076-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Sat Nov 22 15:36:05 2014
Return-Path: <gcc-bugs-return-468076-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 13763 invoked by alias); 22 Nov 2014 15:36:05 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 13717 invoked by uid 48); 22 Nov 2014 15:36:01 -0000
From: "fxcoudert at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug fortran/64022] [F2003][IEEE] ieee_support_flag does not handle kind\x10 and kind\x16 REAL variables
Date: Sat, 22 Nov 2014 15:36:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: fortran
X-Bugzilla-Version: 5.0
X-Bugzilla-Keywords: rejects-valid
X-Bugzilla-Severity: normal
X-Bugzilla-Who: fxcoudert at gcc dot gnu.org
X-Bugzilla-Status: NEW
X-Bugzilla-Priority: P3
X-Bugzilla-Assigned-To: fxcoudert at gcc dot gnu.org
X-Bugzilla-Target-Milestone: 5.0
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields: bug_status cf_reconfirmed_on assigned_to target_milestone everconfirmed
Message-ID: <bug-64022-4-EKPMOiXk2s@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-64022-4@http.gcc.gnu.org/bugzilla/>
References: <bug-64022-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2014-11/txt/msg02548.txt.bz2
Content-length: 809

https://gcc.gnu.org/bugzilla/show_bug.cgi?idd022

Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2014-11-22
           Assignee|unassigned at gcc dot gnu.org      |fxcoudert at gcc dot gnu.org
   Target Milestone|---                         |5.0
     Ever confirmed|0                           |1

--- Comment #1 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
I was under the impression that if IEEE_SUPPORT_DATATYPE is false for a given
kind, then other functions need to be available. Reading the standard again, I
was probably wrong.

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[gcc-bugzilla at mkarcher dot dialup.fu-berlin.de]

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

--- Comment #22 from Michael Karcher <gcc-bugzilla at mkarcher dot dialup.fu-berlin.de> ---
OK, in that case I retract my objections and I think the patch is fine. I am
sorry for that mistake.

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[olegendo at gcc dot gnu.org]

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

--- Comment #23 from Oleg Endo <olegendo at gcc dot gnu.org> ---
Author: olegendo
Date: Sat Nov 22 15:50:10 2014
New Revision: 217969

URL: https://gcc.gnu.org/viewcvs?rev=217969&root=gcc&view=rev
Log:
gcc/
    PR target/63783
    PR target/51244
    * config/sh/sh_treg_combine.cc (sh_treg_combine::make_not_reg_insn):
    Do not emit bitwise not insn.  Emit logical not insn sequence instead.
    Adjust related comments throughout the file.

gcc/testsuite/
    PR target/63783
    PR target/51244
    * gcc.target/sh/torture/pr63783-1.c: New.
    * gcc.target/sh/torture/pr63783-2.c: New.
    * gcc.target/sh/pr51244-20.c: Adjust.
    * gcc.target/sh/pr51244-20-sh2a.c: Adjust.

Added:
    trunk/gcc/testsuite/gcc.target/sh/torture/pr63783-1.c
    trunk/gcc/testsuite/gcc.target/sh/torture/pr63783-2.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/sh/sh_treg_combine.cc
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.target/sh/pr51244-20-sh2a.c
    trunk/gcc/testsuite/gcc.target/sh/pr51244-20.c

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[olegendo at gcc dot gnu.org]

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

--- Comment #24 from Oleg Endo <olegendo at gcc dot gnu.org> ---
Author: olegendo
Date: Sat Nov 22 16:07:25 2014
New Revision: 217970

URL: https://gcc.gnu.org/viewcvs?rev=217970&root=gcc&view=rev
Log:
gcc/
    Backport from mainline
    2014-11-22  Oleg Endo  <olegendo@gcc.gnu.org>

    PR target/63783
    PR target/51244
    * config/sh/sh_treg_combine.cc (sh_treg_combine::make_not_reg_insn):
    Do not emit bitwise not insn.  Emit logical not insn sequence instead.
    Adjust related comments throughout the file.

gcc/testsuite/
    Backport from mainline
    2014-11-22  Oleg Endo  <olegendo@gcc.gnu.org>

    PR target/63783
    PR target/51244
    * gcc.target/sh/torture/pr63783-1.c: New.
    * gcc.target/sh/torture/pr63783-2.c: New.
    * gcc.target/sh/pr51244-20.c: Adjust.
    * gcc.target/sh/pr51244-20-sh2a.c: Adjust.

Added:
    branches/gcc-4_9-branch/gcc/testsuite/gcc.target/sh/torture/pr63783-1.c
    branches/gcc-4_9-branch/gcc/testsuite/gcc.target/sh/torture/pr63783-2.c
Modified:
    branches/gcc-4_9-branch/gcc/ChangeLog
    branches/gcc-4_9-branch/gcc/config/sh/sh_treg_combine.cc
    branches/gcc-4_9-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_9-branch/gcc/testsuite/gcc.target/sh/pr51244-20-sh2a.c
    branches/gcc-4_9-branch/gcc/testsuite/gcc.target/sh/pr51244-20.c

[Bug target/63783] [4.9/5 Regression] [SH] Miscompilation of boolean negation on SH4 using -O2

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #25 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed I assume.