[Bug middle-end/110515] [12/13/14 Regression] llvm-15.0.7 possibly invalid code on -O3

["rguenth at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2023-07-05
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Sergei Trofimovich from comment #7)
> (In reply to Richard Biener from comment #6)
> > Turning off PRE also allows it to pass but I guess it's the same as with SRA.
> > -fno-strict-aliasing also helps, -fno-ipa-modref doesn't.  I suspect
> > bisection
> > isn't going to help much, but maybe it points to a small set of changes on
> > the testcase itself.
> 
> Bisect landed on the r12-4790-g4b3a325f07aceb :
> 
> $ git bisect bad
> 4b3a325f07acebf47e82de227ce1d5ba62f5bcae is the first bad commit
> commit 4b3a325f07acebf47e82de227ce1d5ba62f5bcae
> Author: Aldy Hernandez <aldyh@redhat.com>
> Date:   Thu Oct 28 15:35:21 2021 +0200
> 
>     Remove VRP threader passes in exchange for better threading pre-VRP.

After this commit the bug still appears with -fdisable-tree-threadfull1
-fdisable-tree-threadfull2 and before the commit it appears with
-fdisable-tree-vrp-thread1 -fdisable-tree-vrp-thread2 so it seems
removing the threading passes after VRP exposes the issue.

Looking at the source and the IL of grow() I think the issue is that
loop invariant motion hoists _134 and _104 in

  _31 = Visited.Small;
  if (_31 != 0)
    goto <bb 5>; [50.00%]
  else
    goto <bb 4>; [50.00%]

  <bb 4> [local count: 621159684]:
  _145 = MEM[(struct LargeRep *)&Visited + 8B].Slots;
  _34 = MEM[(struct LargeRep *)&Visited + 8B].Capacity;
  if (_34 != 0)
    goto <bb 5>; [0.00%]
  else
    goto <bb 9>; [0.00%]

  <bb 5> [local count: 955630290]:
  # _134 = PHI <_34(4), 2(3)>
  # _104 = PHI <_145(4), &Visited.u.i(3)>

  <bb 6> [local count: 8933211531]:

out of the inner first loop which I think is OK even though it makes
the _145 and _34 reads unconditional.  Then PRE hoists those further,
but correctly identifying

  Visited.Small = 0;
  _94 = operator new [] (1024);
  Visited.u.o.Slots = _94;
  Visited.u.o.Capacity = 128;

as where they change.

Interestingly I can disable (almost) all passes after tree PRE and still
get the failure.  One change PRE does is

-  _18 = MEM[(struct V *)&Visited + 8B].v;
-  if (_18 != 790526)
-    goto <bb 26>; [66.00%]
+  _196 = (long unsigned int) prephitmp_173;
+  if (prephitmp_173 != 790526B)
+    goto <bb 22>; [66.00%]

where it replaces the read with _94 (or EmptyKey as from entry).  That's
phishy.  It's the != EmptyKey check in grow when copying inline slots into
temporary storage.

So on 4b3a325f07acebf47e82de227ce1d5ba62f5bcae^ (one before the bisected
rev.) it fails with

g++ t.C -O2 -fdisable-tree-vrp-thread1 -fdump-tree-pre-details -fdump-tree-all
-fno-tree-tail-merge -fno-tree-vectorize -fno-tree-loop-optimize
-fdisable-tree-sink1 -fdisable-tree-dse3 -fdisable-tree-dce4
-fdisable-tree-fre5 -fdisable-tree-dom3 -fdisable-tree-thread3
-fdisable-tree-thread4 -fdisable-tree-vrp2 -fdisable-tree-vrp-thread2
-fdisable-tree-dse5 -fdisable-tree-cddce3 -fdisable-tree-sink2
-fdisable-tree-store-merging -fdisable-tree-dce7 -fdisable-tree-modref2
-fdisable-tree-local-pure-const2 -fdisable-tree-split-paths -fdisable-tree-ccp4
-fdisable-tree-slsr -fdisable-tree-reassoc2 -fdisable-tree-switchlower1
-fdisable-tree-forwprop4 -fdisable-tree-phiopt4 -fno-gcse -fno-dse
-fno-gcse-after-reload -fdbg-cnt=treepre_insert:1-3:16-21:24-24
-fno-tree-partial-pre

So it goes that the above load is CSEd to the LIM inserted load plus
a conversion from pointer to unsigned long:

Value numbering stmt = _145 = MEM[(struct LargeRep *)&Visited + 8B].Slots;
Setting value number of _145 to _145 (changed)
...
Value numbering stmt = _18 = MEM[(struct V *)&Visited + 8B].v;
Inserting name _47 for expression (long unsigned int) _145
Setting value number of _18 to _47 (changed)

we seem to disregard TBAA when CSEing two loads without an intervening
possibly aliasing stores.  But this then causes PRE (well, that's a
long-long-standing issue...) to record the later load to EXP_GEN as

exp_gen[25] := {
{component_ref<Slots>,mem_ref<8B>,addr_expr<&Visited>}@.MEM_129 (0084), _18
(0031) }

so to PRE it appears that we do

  _18 = (long unsigned int) MEM[(struct LargeRep *)&Visited + 8B].Slots;

and thus the wrong ref is used for disambiguation which means we
disambiguate against

  TheSlot_172->v = Item$f_20;

which eventually stores to that slot.

*sigh*