Bug 116057 - [12 Regression] SLP can introduces partialized uninitialized vectors
Summary: [12 Regression] SLP can introduces partialized uninitialized vectors
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 14.1.1
: P2 normal
Target Milestone: 12.5
Assignee: Richard Biener
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2024-07-23 18:26 UTC by Sasha Finkelstein
Modified: 2025-01-10 14:45 UTC (History)
4 users (show)

See Also:
Host:
Target: aarch64-unknown-linux-gnu x86_64-linux-gnu riscv*-*-*
Build:
Known to work: 12.4.1, 13.3.1, 14.1.1, 15.0
Known to fail: 12.4.0, 13.3.0, 14.1.0
Last reconfirmed: 2024-07-23 00:00:00


Attachments
Reduced example. (177 bytes, text/plain)
2024-07-23 18:26 UTC, Sasha Finkelstein
Details
Full preprocessed source part 1 (512.08 KB, application/x-troff-man)
2024-07-24 18:17 UTC, Sasha Finkelstein
Details
Full preprocessed source part 2 (880.80 KB, application/x-troff-man)
2024-07-24 18:17 UTC, Sasha Finkelstein
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sasha Finkelstein 2024-07-23 18:26:14 UTC
Created attachment 58741 [details]
Reduced example.

Attempting to compile the attached file with "g++ -pthread -O1 -ftree-slp-vectorize -pipe -std=gnu++20 -fno-stack-protector" on arm64 results in a miscompilation where ObjectWriteToReadOnlyProperty returns 1 in x0 on Throw() path. Removing '-ftree-slp-vectorize' results in correct output.
This is a reduced example that was extraced from nodejs code.
On arm64 specifically, this is a regression caused by 33c2b70dbabc02788caabcbc66b7baeafeb95bcf, however as that commit only changes costs, the bug is most likely somewhere else.
Comment 1 Arsen Arsenović 2024-07-23 18:35:48 UTC
I've debugged this already AOT.

SLP generates the following:

  <bb 2> [local count: 1073741824]:
  _8 = {0, __trans_tmp_1$1_4(D)};
  WriteToReadOnlyProperty_should_throw.0_1 = WriteToReadOnlyProperty_should_throw;
  if (WriteToReadOnlyProperty_should_throw.0_1 != 0)
    goto <bb 4>; [67.00%]
  else
    goto <bb 3>; [33.00%]

  <bb 3> [local count: 354334800]:
  Throw ();

  <bb 4> [local count: 1073741824]:
  # _10 = PHI <0(2), __trans_tmp_1$1_4(D)(3)>
  # _12 = PHI <1(2), 0(3)>
  # vect__12.9_6 = PHI <{ 1, 0 }(2), _8(3)>
  MEM[(struct Maybe *)&D.4540] = vect__12.9_6;
  return D.4540;

... from:

  <bb 2> [local count: 1073741824]:
  WriteToReadOnlyProperty_should_throw.0_1 = WriteToReadOnlyProperty_should_throw;
  if (WriteToReadOnlyProperty_should_throw.0_1 != 0)
    goto <bb 4>; [67.00%]
  else
    goto <bb 3>; [33.00%]

  <bb 3> [local count: 354334800]:
  Throw ();

  <bb 4> [local count: 1073741824]:
  # _10 = PHI <0(2), __trans_tmp_1$1_4(D)(3)>
  # _12 = PHI <1(2), 0(3)>
  MEM <unsigned char> [(struct Maybe *)&D.4540] = _12;
  MEM <unsigned char> [(struct Maybe *)&D.4540 + 1B] = _10;
  return D.4540;

... replacing the two MEMs with a vector.

later, ccp4 decides:

Visiting statement:
_8 = {0, __trans_tmp_1$1_4(D)};
which is likely UNDEFINED
Lattice value changed to UNDEFINED.  Adding SSA edges to worklist.
marking stmt to be not simulated again
...
Visiting PHI node: vect__12.9_6 = PHI <{ 1, 0 }(2), _8(3)>
	Argument #0 (2 -> 4 executable)
	{ 1, 0 }	Value: CONSTANT { 1, 0 }
	Argument #1 (3 -> 4 executable)
	_8	Value: UNDEFINED

    PHI node value: CONSTANT { 1, 0 }
...

  <bb 2> [local count: 1073741824]:
  WriteToReadOnlyProperty_should_throw.0_1 = WriteToReadOnlyProperty_should_throw;
  if (WriteToReadOnlyProperty_should_throw.0_1 != 0)
    goto <bb 4>; [67.00%]
  else
    goto <bb 3>; [33.00%]

  <bb 3> [local count: 354334800]:
  Throw ();

  <bb 4> [local count: 1073741824]:
  # _12 = PHI <1(2), 0(3)>
  MEM[(struct Maybe *)&D.4540] = { 1, 0 };
  return D.4540;

and, so, as a result both branches start returning { 1, 0 }.
Comment 2 Andrew Pinski 2024-07-23 18:37:48 UTC
g:33c2b70dbabc02788caabcbc66b7baeafeb95bcf

That just changed the cost model.

Most likely could be reproduced with -fno-cost-model beforehand. 

Note there is an unitialized usage on one side of the if so that might be an iasue.
Comment 3 Andrew Pinski 2024-07-23 18:47:12 UTC
#define vect8 __attribute__((vector_size(8)))

vect8 int f(int a)
{
  int b;
  vect int t={1,1};
  if(a) return t;
  return (vect8 int){0, b};
}
Comment 4 Andrew Pinski 2024-07-23 18:59:48 UTC
So this code might have undefined behavior. At the very least it is questionable.
Comment 5 Andrew Pinski 2024-07-23 19:00:44 UTC
Can you attach the original code that was failing since the reduced one is questionable?
Comment 7 Andrew Pinski 2024-07-23 19:32:16 UTC
Can you attach the full preprocessed source instead of just linking?
Comment 8 Andrew Pinski 2024-07-23 19:39:22 UTC
The question comes is this defined or undefined?
I think it is still undefined.

Changing:
Maybe() : has_value_(false) {}

into:
Maybe() : has_value_(false), value_() {}

Makes this well defined.
And has no effect on if value_ had a constructor or not.
Comment 9 Andrew Pinski 2024-07-23 20:36:49 UTC
So r12-397-gda9e6e63d1ae22 introduced ccp after the slp vectorizer.

Also it happens on x86_64 with -fno-vect-cost-model.
Comment 10 Sam James 2024-07-24 00:16:25 UTC
Other links for completeness (don't fit in See Also right now, need to ping overseers about applying the patches for that...):
* https://issues.chromium.org/issues/42203868
* https://issues.chromium.org/issues/42204492
Comment 11 Richard Biener 2024-07-24 11:07:31 UTC
I think the bug is in CCPs likely_value, it doesn't see the constant operand in the CTOR but only the undefined one and thus makes it UNDEFINED instead of CONSTNAT.
Comment 12 GCC Commits 2024-07-24 12:09:57 UTC
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:1ea551514b9c285d801ac5ab8d78b22483ff65af

commit r15-2255-g1ea551514b9c285d801ac5ab8d78b22483ff65af
Author: Richard Biener <rguenther@suse.de>
Date:   Wed Jul 24 13:16:35 2024 +0200

    tree-optimization/116057 - wrong code with CCP and vector CTORs
    
    The following fixes an issue with CCPs likely_value when faced with
    a vector CTOR containing undef SSA names and constants.  This should
    be classified as CONSTANT and not UNDEFINED.
    
            PR tree-optimization/116057
            * tree-ssa-ccp.cc (likely_value): Also walk CTORs in stmt
            operands to look for constants.
    
            * gcc.dg/torture/pr116057.c: New testcase.
Comment 13 Richard Biener 2024-07-24 12:11:32 UTC
This fixed the CCP issue.  I think there's a SLP issue as well in that it
inserts the uninit use where it wasn't used before.  Ideally we'd insert
this invariant only on the edge 3->4 - I will see if I can fix that as well.
Comment 14 Sasha Finkelstein 2024-07-24 18:17:13 UTC
Created attachment 58750 [details]
Full preprocessed source part 1
Comment 15 Sasha Finkelstein 2024-07-24 18:17:35 UTC
Created attachment 58751 [details]
Full preprocessed source part 2
Comment 16 GCC Commits 2024-07-29 13:40:16 UTC
The releases/gcc-14 branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:a7f1b00ed69810ce7f000d385a60e148d0228d48

commit r14-10520-ga7f1b00ed69810ce7f000d385a60e148d0228d48
Author: Richard Biener <rguenther@suse.de>
Date:   Wed Jul 24 13:16:35 2024 +0200

    tree-optimization/116057 - wrong code with CCP and vector CTORs
    
    The following fixes an issue with CCPs likely_value when faced with
    a vector CTOR containing undef SSA names and constants.  This should
    be classified as CONSTANT and not UNDEFINED.
    
            PR tree-optimization/116057
            * tree-ssa-ccp.cc (likely_value): Also walk CTORs in stmt
            operands to look for constants.
    
            * gcc.dg/torture/pr116057.c: New testcase.
    
    (cherry picked from commit 1ea551514b9c285d801ac5ab8d78b22483ff65af)
Comment 17 GCC Commits 2024-09-18 10:33:36 UTC
The releases/gcc-13 branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:ef25f1dd600cc9351c80e3e018d7170e16a2c6ff

commit r13-9042-gef25f1dd600cc9351c80e3e018d7170e16a2c6ff
Author: Richard Biener <rguenther@suse.de>
Date:   Wed Jul 24 13:16:35 2024 +0200

    tree-optimization/116057 - wrong code with CCP and vector CTORs
    
    The following fixes an issue with CCPs likely_value when faced with
    a vector CTOR containing undef SSA names and constants.  This should
    be classified as CONSTANT and not UNDEFINED.
    
            PR tree-optimization/116057
            * tree-ssa-ccp.cc (likely_value): Also walk CTORs in stmt
            operands to look for constants.
    
            * gcc.dg/torture/pr116057.c: New testcase.
    
    (cherry picked from commit 1ea551514b9c285d801ac5ab8d78b22483ff65af)
Comment 18 GCC Commits 2025-01-10 14:44:04 UTC
The releases/gcc-12 branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:c8b549857d968d634a74709112e5acc9f9caf35c

commit r12-10895-gc8b549857d968d634a74709112e5acc9f9caf35c
Author: Richard Biener <rguenther@suse.de>
Date:   Wed Jul 24 13:16:35 2024 +0200

    tree-optimization/116057 - wrong code with CCP and vector CTORs
    
    The following fixes an issue with CCPs likely_value when faced with
    a vector CTOR containing undef SSA names and constants.  This should
    be classified as CONSTANT and not UNDEFINED.
    
            PR tree-optimization/116057
            * tree-ssa-ccp.cc (likely_value): Also walk CTORs in stmt
            operands to look for constants.
    
            * gcc.dg/torture/pr116057.c: New testcase.
    
    (cherry picked from commit 1ea551514b9c285d801ac5ab8d78b22483ff65af)
Comment 19 Richard Biener 2025-01-10 14:45:08 UTC
Fixed.