Bug 34048 - [4.3 Regression]: Revision 130040 miscompiles 450.soplex
[4.3 Regression]: Revision 130040 miscompiles 450.soplex
Status: RESOLVED INVALID
Product: gcc
Classification: Unclassified
Component: tree-optimization
4.3.0
: P3 normal
: ---
Assigned To: Not yet assigned to anyone
: alias, wrong-code
Depends on:
Blocks: 33604
  Show dependency treegraph
 
Reported: 2007-11-10 07:29 UTC by H.J. Lu
Modified: 2007-11-11 12:20 UTC (History)
5 users (show)

See Also:
Host:
Target: x86_64-unknown-linux-gnu
Build:
Known to work:
Known to fail:
Last reconfirmed: 2007-11-10 13:26:25


Attachments
patch for 450.soplex (1.17 KB, text/plain)
2007-11-10 16:43 UTC, Richard Biener
Details
A patch (1.64 KB, patch)
2007-11-10 19:42 UTC, H.J. Lu
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description H.J. Lu 2007-11-10 07:29:12 UTC
Revision 130040 miscompiles 450.soplex in SPEC CPU 2006 with -O2 -ffast-math
on Linux/x86-64:

  Running (#1) 450.soplex ref base o2 default

450.soplex: copy #0 non-zero return code (rc=0, signal=11)

Error with '/export/spec/src/2006/x86_64/spec/bin/specinvoke -E -d /export/spec/src/2006/x86_64/spec/benchspec/CPU2006/450.soplex/run/run_base_ref_o2.0000 -c 1 -e compare.err -o compare.stdout -f compare.cmd': check file '/export/spec/src/2006/x86_64/spec/benchspec/CPU2006/450.soplex/run/run_base_ref_o2.0000/.err'
*** Miscompare of ref.stderr, see /export/spec/src/2006/x86_64/spec/benchspec/CPU2006/450.soplex/run/run_base_ref_o2.0000/ref.stderr.mis
*** Miscompare of ref.out, see /export/spec/src/2006/x86_64/spec/benchspec/CPU2006/450.soplex/run/run_base_ref_o2.0000/ref.out.mis
Invalid run; unable to continue.  If you wish to ignore errors please use '-I' or ignore_errors
Comment 1 Richard Biener 2007-11-10 13:26:25 UTC
soplex.o is miscompiled.
Comment 2 Richard Biener 2007-11-10 13:35:28 UTC
All the tree difference is in

virtual void soplex::SoPlex::reDim() (this)

and it looks like an alias issue (what else...)
Comment 3 Richard Biener 2007-11-10 14:31:40 UTC
tree-ssa-sink is the culprit (we re-compute aliasing after PRE, right before sink,
so an error there might be the bug as well):

Sinking newdim_31 = newdim_1 + -1 from bb 15 to bb 12
Sinking # SFT.2356_213 = VDEF <SFT.2356_209>
D.45672_138->idx = D.45677_152 from bb 12 to bb 14
Sinking D.45677_152 = prephitmp.2402_141 from bb 12 to bb 14
Sinking # SFT.2356_209 = VDEF <SFT.2356_197>
D.45676_147->val = 1.0e+0 from bb 12 to bb 14
Sinking # SFT.2355_200 = VDEF <SFT.2355_186>
D.45672_138->val = 1.0e+0 from bb 10 to bb 14
Sinking # SFT.2354_136 = VDEF <SFT.2354_139>
D.37036.D.34678.m_elem = D.45669_134 from bb 10 to bb 14
Sinking # SFT.2355_186 = VDEF <SFT.2355_161>
D.37036.themem.val = 0.0 from bb 10 to bb 14
virtual void soplex::SoPlex::reDim() (this)

at least ptr->val with only a single SFT looks suspicious.
Comment 4 Richard Biener 2007-11-10 14:36:24 UTC
<bb 10>:
  # SFT.2355_186 = VDEF <SFT.2355_161>
  D.37036.themem.val = 0.0;
  D.45669_134 = &D.37036.themem + 16;
...

<bb 11>:
  # VUSE <SFT.2356_197>
  n_142 = D.45672_138->idx;
  pretmp.2399_154 = (long unsigned int) n_142;
  pretmp.2399_151 = pretmp.2399_154 * 16;
  pretmp.2401_143 = n_142 + 1;

<bb 12>:
  # prephitmp.2402_141 = PHI <1(22), pretmp.2401_143(11)>
  # prephitmp.2400_148 = PHI <0(22), pretmp.2399_151(11)>
  # prephitmp.2400_153 = PHI <0(22), pretmp.2399_154(11)>
  # n_145 = PHI <0(22), n_142(11)>
  D.45674_144 = prephitmp.2400_153;
  D.45675_146 = prephitmp.2400_148;
  D.45676_147 = D.45669_134 + D.45675_146;
  # SFT.2357_206 = VDEF <SFT.2357_166>
  D.45676_147->idx = newdim_31;
  # SFT.2356_209 = VDEF <SFT.2356_197>
  D.45676_147->val = 1.0e+0;

so D.45676_147->val cannot just alias with SFT.2356.
Comment 5 Richard Biener 2007-11-10 16:09:26 UTC
Again aliasing is completely dependent on whether unrelated functions are present or not.  *sigh*
Comment 6 Richard Biener 2007-11-10 16:42:41 UTC
This looks like a bug in soplex.  It does:

class UnitVector : public SVector
{
private:
   Element themem;
   Element themem1; 

public:
   
...
   UnitVector(int i = 0)
      : SVector(2, &themem)
   {  
      add(i, 1.0);
   }

and:

class SVector
{
...
   struct Element
   {  
      Real val;
      int idx;
   };
   Element *m_elem;
...
   SVector(int n = 0, Element* p_mem = 0)
   {  
      setMem(n, p_mem);
   }
...
   void setMem(int n, Element* elmem)
   {  
      (static_cast<void> (0));

      if (n > 0)
      {
         (static_cast<void> (0));
         elmem->val = 0;
         m_elem = &(elmem[1]);
         set_size( 0 );
         set_max ( n - 1 );
      }
      else
         m_elem = 0;
   }

that is, SVector accesses themem, themem1 as array (obviously to avoid
unneccessary default construction).  If you fix this, the miscompile
goes away.
Comment 7 Richard Biener 2007-11-10 16:43:36 UTC
Created attachment 14525 [details]
patch for 450.soplex
Comment 8 Richard Biener 2007-11-10 16:44:06 UTC
So, invalid.
Comment 9 Paolo Bonzini 2007-11-10 18:10:07 UTC
You should report the problem to SPEC so they update http://www.spec.org/cpu2006/Docs/450.soplex.html and create a src.alt (I think, at least this is how it was for CPU2000).
Comment 10 rguenther@suse.de 2007-11-10 18:20:26 UTC
Subject: Re:  [4.3 Regression]: Revision 130040
 miscompiles 450.soplex

On Sat, 10 Nov 2007, bonzini at gnu dot org wrote:

> ------- Comment #9 from bonzini at gnu dot org  2007-11-10 18:10 -------
> You should report the problem to SPEC so they update
> http://www.spec.org/cpu2006/Docs/450.soplex.html and create a src.alt (I think,
> at least this is how it was for CPU2000).

I'll leave that to HJ who has done so in the past.

Richard.
Comment 11 H.J. Lu 2007-11-10 18:41:07 UTC
(In reply to comment #10)
> Subject: Re:  [4.3 Regression]: Revision 130040
>  miscompiles 450.soplex
> 
> On Sat, 10 Nov 2007, bonzini at gnu dot org wrote:
> 
> > ------- Comment #9 from bonzini at gnu dot org  2007-11-10 18:10 -------
> > You should report the problem to SPEC so they update
> > http://www.spec.org/cpu2006/Docs/450.soplex.html and create a src.alt (I think,
> > at least this is how it was for CPU2000).
> 
> I'll leave that to HJ who has done so in the past.
> 

I will take care of it.  Thanks, Richard.

Comment 12 H.J. Lu 2007-11-10 19:17:14 UTC
(In reply to comment #7)
> Created an attachment (id=14525) [edit]
> patch for 450.soplex
> 

This patch is incomplete. I got

unitvector.cc: In member function 'bool soplex::UnitVector::isConsistent() const':
unitvector.cc:30: error: comparison between distinct pointer types 'soplex::SVector::Element*' and 'const soplex::SVector::Element (*)[2]' lacks a cast
unitvector.cc:32: error: 'themem1' was not declared in this scope
specmake[1]: *** [unitvector.o] Error 1
Comment 13 H.J. Lu 2007-11-10 19:42:32 UTC
Created attachment 14526 [details]
A patch

This patch seems to work.
Comment 14 Paolo Bonzini 2007-11-11 07:16:18 UTC
It could also be possible to do something like this to avoid default construction.

@@ -46,8 +46,8 @@ namespace soplex
 class UnitVector : public SVector
 {
 private:
-   Element themem;  ///< memory for 1st sparse vector entry (size)
-   Element themem1; ///< memory for 2nd sparse vector entry (idx,1.0)
+   char themem[sizeof(Element[2])];  ///< memory for 1st sparse vector entry (size)
+   /*Element themem1;*/ ///< memory for 2nd sparse vector entry (idx,1.0)
 
 public:
    /// returns value = 1
@@ -62,23 +62,22 @@ public:
 
    /// construct \c i 'th unit vector.
    UnitVector(int i = 0)
-      : SVector(2, &themem)
+      : SVector(2, reinterpret_cast<Element *> (themem))
    {
-      add(i, 1.0);
+      new(themem[0]) Element (); new (themem[sizeof (Element)]) Element (); add(i, 1.0);
    }
 
    ///  copy constructor
    UnitVector(const UnitVector& rhs)
-      : SVector(2, &themem)
-      , themem (rhs.themem)
-      , themem1(rhs.themem1)
-   {}
+      : SVector(2, reinterpret_cast<Element *> (themem))
+   { new(themem[0]) Element (reinterpret_cast<Element *> (rhs.themem[0]));
+     new(themem[sizeof (Element)]) Element (reinterpret_cast<Element *> (rhs.themem[sizeof (Element)]));  }
 
    /// assignment
    UnitVector& operator=(const UnitVector& rhs)
    {
       if ( this != &rhs )
-         themem1 = rhs.themem1;
+         new(themem[sizeof (Element)]) Element (reinterpret_cast<Element *> (rhs.themem[sizeof (Element)]));
       return *this;
    }
 

Not the cleanest possible code, but I doubt SPEC will like to introduce a src.alt that makes previous reports not comparable with the new ones.
Comment 15 Richard Biener 2007-11-11 12:19:46 UTC
Well, gcc is able to optimize the two-element array case the same.  Also you
still do default construction with the placement new (if Element has a default
constructor).  Maybe Element is POD, in which case there is no default
construction for the array case either (I didn't really investigate but only
speculated about the reason for the strange code).
Comment 16 Richard Biener 2007-11-11 12:20:19 UTC
Closing as invalid again ;)