SimplifyCFG removes needed path of computed goto

[FlameTop]

We have determined a case where required code in a computed goto flow is removed incorrectly. The included source has been cut-down from a more complex example and appears to be the simplest we can create to reproduce the fault. In the code the [0]th part of the computed goto is removed and replaced by the [2]nd path of the goto (can be determined by the the constant -0XDEADBEEF being absent from the assembly output).

It appears the commit
@

commit fc6bdb8
Author: XChy [email protected]
Date: Sat Oct 28 17:10:20 2023 +0800

[SimplifyCFG] Reland transform for redirecting phis between unmergeable BB and SuccBB (#68473)

Reland #67275 with #68953 resolved.

introduced this regression.

Small.CPP: '-S -O2' will reproduce.

#typedef unsigned char uint8_t;
typedef unsigned short int uint16_t;
typedef short int int16_t;
typedef unsigned int uint32_t;
typedef int int32_t;
typedef unsigned long long uint64_t;
using u8    = uint8_t;
using s16   = int16_t;
using u16   = uint16_t;
using s32   = int32_t;
using u32   = uint32_t;
union u128 {
  struct
  {
    uint32_t  w0, w1;
    uint32_t  w2, w3;
  };
  struct
  {
    uint64_t d0;
    uint64_t d1;
  };
};
namespace IDX{
enum _enum_t : u8
{
  x0 = 0,
  x1,    x2,
  x3,    x4,    x5,

  Invalid,
};
}


template< typename T >
union eState_t
{
  struct
  {
    T x0, x1, x2,
      x3, x4, x5;
  };
  T   m_r[IDX::Invalid];
};

using Pack128 = u128;
using eState128 = eState_t<Pack128>;
struct State
{
  eState128  sdta; 
};



void qaz(uint8_t *mem, State &__restrict data){
  uint32_t switchval = ~0;
  data.sdta.m_r[(IDX::x5)].d0 = data.sdta.m_r[(IDX::x3)].d0;
  switchval = data.sdta.m_r[(IDX::x2)].d0;
  static constexpr void *switchdispatch[] = {
    &&L10,
    &&L20,
    &&L30,
    &&L40,
  };
  goto *switchdispatch[switchval];
  return;

L10:
  data.sdta.m_r[(IDX::x3)].d0 = 0xDEADBEEF;
  goto L200;
L20:
  if (auto reason = (data.sdta.m_r[(IDX::x1)].d0) == (data.sdta.m_r[(IDX::x0)].d0); (data.sdta.m_r[(IDX::x1)].d0 = (0x005b << 16)), reason) { goto L100; };
  if (auto reason = (0 == 0); (data.sdta.m_r[(IDX::x3)].d0 = (int32_t((data.sdta.m_r[(IDX::x1)].w0) + 0x5d00))), reason) { goto L200; };
L30:
  data.sdta.m_r[(IDX::x1)].d0 = (0x005b << 16) ;
  if (auto reason = (0 == 0); (data.sdta.m_r[(IDX::x3)].d0 = (int32_t((data.sdta.m_r[(IDX::x1)].d0) + 0x5d40))), reason) { goto L200; };
L40:
  if(auto reason = ((data.sdta.m_r[(IDX::x1)].d0) != ( 0 )); reason) { (data.sdta.m_r[(IDX::x3)].d0 = (s32)((s32 &)mem[(u32)(((IDX::x5)) + (0x398))])); goto L200; };
L100:
  data.sdta.m_r[(IDX::x1)].d0 = (0x005b << 16);
  data.sdta.m_r[(IDX::x3)].d0 = data.sdta.m_r[(IDX::x1)].w0 + 0x5ce0;
L200:
  if(auto reason = ((data.sdta.m_r[(IDX::x3)].d0) == ( 0 )); data.sdta.m_r[(IDX::x1)].d0 = ((data.sdta.m_r[(IDX::x3)].d0) + 0), reason) { goto L300; };
  data.sdta.m_r[(IDX::x2)].d0 = (uint32_t)(uint32_t((u16)((s16 &)mem[(u32)(( data.sdta.m_r[(IDX::x3)].d0) + (0xa))])));
L300:
  return;

}

[nikic]

Do you see this on current main? This looks like the issue 3c9022c fixed.

[FlameTop]

I am still seeing the problem and have confirmed commit 3c9022c is present and correct in my build. I am also seeing the problem on Compiler Explorer trunk build

'''clang version 20.0.0git (https://github.com/llvm/llvm-project.git 4527894)'''

which appears to be up to date as of 26th November 2024, so do not think this is a local build problem.

[nikic]

Can you share the compiler explorer link? In https://clang.godbolt.org/z/z58j37661 I see the constant 3735928559.

[FlameTop]

Argh, apologies I had the wrong explorer page open and was looking at the LLVM 19 results (in which it is broken). I can now see that compiler explorer trunk build does work ok. I need to dig and find why my local build of main still exhibits the problem.

[nikic]

There was a followup fix yesterday in 39601a6, so maybe your local build doesn't have that one?

[nikic]

For the record, #117869 is the backport PR.

[FlameTop]

Ah, thank you I did not have yesterday's follow-up fix. Rebuilding now to confirm my local build works.

[FlameTop]

I have confirmed the fix 39601a6 from yesterday does fix my local build.

Again apologies for wasting your time; just my luck to report a bug the day its fixed.

regards

Phil

[nikic]

Heh, no luck involved -- the original fix only got a second look because of this report :)