[Bug target/65773] New: [5.1 regression] GCC 5.1 miscompiles LLVM function AArch64InstrInfo::loadRegFromStackSlot()

[Bug target/65773] New: [5.1 regression] GCC 5.1 miscompiles LLVM function AArch64InstrInfo::loadRegFromStackSlot()

[wschmidt at gcc dot gnu.org]

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

            Bug ID: 65773
           Summary: [5.1 regression] GCC 5.1 miscompiles LLVM function
                    AArch64InstrInfo::loadRegFromStackSlot()
           Product: gcc
           Version: 5.1.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: wschmidt at gcc dot gnu.org
                CC: bergner at gcc dot gnu.org, jakub at gcc dot gnu.org,
                    rguenth at gcc dot gnu.org
              Host: powerpc64*-linux-gnu
            Target: powerpc64*-linux-gnu
             Build: powerpc64*-linux-gnu

I built gcc from branches/gcc-5-branch yesterday and used it to build
Clang/LLVM as an acid test.  Unfortunately the test suite shows a number of
failures.  I've traced these down to a miscompile of
AArch64InstrInfo::loadRegFromStackSlot( <long argument list omitted> ).  The
bad code can be seen here:

    105eb74c:   00 00 60 38     li      r3,0
    105eb750:   28 00 41 f9     std     r10,40(r1)
    105eb754:   e4 06 29 79     rldicr  r9,r9,0,59
    105eb758:   30 00 41 f9     std     r10,48(r1)
    105eb75c:   21 00 01 99     stb     r8,33(r1)
    105eb760:   22 00 21 99     stb     r9,34(r1)
    105eb764:   a5 ec 79 48     bl      10d8a408
<_ZN4llvm12MachineInstr10addOp\
erandERNS_15MachineFunctionERKNS_14MachineOperandE+0x8>

This calls MachineInstr::addOperand(MachineFunction&, MachineOperand const&)
with r3 as the this pointer.  Loading zero into the this pointer ahead of the
call seems somewhat unhelpful.

The same code compiled with the 4.8 gcc I usually use shows:

    106a8fa0:   78 cb 25 7f     mr      r5,r25
    106a8fa4:   78 f3 c3 7f     mr      r3,r30
    106a8fa8:   20 00 9e eb     ld      r28,32(r30)
    106a8fac:   78 eb a4 7f     mr      r4,r29
    106a8fb0:   78 d3 46 7f     mr      r6,r26
    106a8fb4:   65 d4 7c 48     bl      10e76418
<_ZNK4llvm12MachineInstr21getR\
egClassConstraintEjPKNS_15TargetInstrInfoEPKNS_18TargetRegisterInfoE+0x8>

which is clearly much more reasonable.

I'll gather reproduction materials, but wanted to get the bug filed quickly for
awareness.

[Bug target/65773] [5 Regression] GCC 5.1 miscompiles LLVM function AArch64InstrInfo::loadRegFromStackSlot()

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2015-04-15
            Summary|[5.1 regression] GCC 5.1    |[5 Regression] GCC 5.1
                   |miscompiles LLVM function   |miscompiles LLVM function
                   |AArch64InstrInfo::loadRegFr |AArch64InstrInfo::loadRegFr
                   |omStackSlot()               |omStackSlot()
     Ever confirmed|0                           |1

[Bug target/65773] [5 Regression] GCC 5.1 miscompiles LLVM function AArch64InstrInfo::loadRegFromStackSlot()

[wschmidt at gcc dot gnu.org]

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

--- Comment #1 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
Well, I screwed up, the "good" code is calling a different function.  In the
good code this function call was apparently inlined, so I can't point to it. 
But still, the load of r3 with zero is a bad thing.

[Bug target/65773] [5 Regression] GCC 5.1 miscompiles LLVM function AArch64InstrInfo::loadRegFromStackSlot()

[wschmidt at gcc dot gnu.org]

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

--- Comment #2 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
Found it...the "real" good code is:

    106a8d6c:   78 fb e3 7f     mr      r3,r31                                  
    106a8d70:   78 db 64 7f     mr      r4,r27                                  
    106a8d74:   20 00 01 99     stb     r8,32(r1)                               
    106a8d78:   00 00 00 39     li      r8,0                                    
    106a8d7c:   78 eb a5 7f     mr      r5,r29                                  
    106a8d80:   28 00 21 f9     std     r9,40(r1)                               
    106a8d84:   36 00 4a 55     rlwinm  r10,r10,0,0,27                          
    106a8d88:   30 00 21 f9     std     r9,48(r1)                               
    106a8d8c:   21 00 01 99     stb     r8,33(r1)                               
    106a8d90:   22 00 41 99     stb     r10,34(r1)                              
    106a8d94:   75 ef 7c 48     bl      10e77d08
<_ZN4llvm12MachineInstr10addOp\
erandERNS_15MachineFunctionERKNS_14MachineOperandE+0x8>

[Bug target/65773] [5 Regression] GCC 5.1 miscompiles LLVM function AArch64InstrInfo::loadRegFromStackSlot()

[wschmidt at gcc dot gnu.org]

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

--- Comment #3 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
Created attachment 35322
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35322&action=edit
Unreduced save-temps file AArch64InstrInfo.ii.gz

Attaching the (unreduced and compressed) preprocessed source.  So far I haven't
been able to reduce it.  The incorrect code generation can be reproduced with:

$ g++ -O2 -std=c++11 -S AArch64InstrInfo.ii

or with

$ g++ -O1 -std=c++11 -S AArch64InstrInfo.ii

At -O0, there are no calls to
_ZN4llvm12MachineInstr10addOperandERNS_15MachineFunctionERKNS_14MachineOperandE
in the text of the caller, so inlining must be present.  All calls to this
function have reasonable setup code for r3, so -O1 is the minimum optimization
level required to reproduce.

The bad code may be found in the function _ZNK4llvm16AArch64InstrInfo20\
loadRegFromStackSlotERNS_17MachineBasicBlockENS1_15bundle_iteratorINS_12Machine\
InstrENS_14ilist_iteratorIS4_EEEEjiPKNS_19TargetRegisterClassEPKNS_18TargetRegi\
sterInfoE, at the third call to
_ZN4llvm12MachineInstr10addOperandERNS_15MachineFunctionERKNS_14MachineOperandE.

[Bug target/65773] [5 Regression] GCC 5.1 miscompiles LLVM function AArch64InstrInfo::loadRegFromStackSlot()

[hubicka at gcc dot gnu.org]

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

Jan Hubicka <hubicka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hubicka at gcc dot gnu.org

--- Comment #4 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Loading NULL as THIS pointer seems fishy indeed. One case I can think of is ICF
merging the method with something else and redirect the call to something else
where parameter 0 has different meaning.

Can you try -fno-icf?

[Bug tree-optimization/65773] [5 Regression] GCC 5.1 miscompiles LLVM function AArch64InstrInfo::loadRegFromStackSlot()

[wschmidt at gcc dot gnu.org]

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

Bill Schmidt <wschmidt at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |NEW
          Component|target                      |tree-optimization

--- Comment #5 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
It is difficult to track down exactly what happened here because there appears
to be some inlining occurring without any dumps showing up with
-fdump-tree-all.  After .046t.inline_param2, we see 

  <bb 56>:
  _71 = llvm::MachineInstrBuilder::addFrameIndex (_69, FI_16(D));

...

  <bb 60>:
  _73 = _71->MI;
  _209 = _71->MF;
  D.381968.OpKind = 1;
  D.381968.SubReg_TargetFlags = 0;
  D.381968.ParentMI = 0B;
  D.381968.Contents.ImmVal = 0;
  llvm::MachineInstr::addOperand (_73, _209, &D.381968);

By the time we get to the next dump (.068t.fixup_cfg4) there has been some
inline expansion performed.  The relevant computations are:

  <bb 113>:
  D.383141 ={v} {CLOBBER};
  _428 = &D.324881;
  _69 = _428;
  _354 = _69->MI;
  _355 = _69->MF;
  D.383161.OpKind = 5;
  D.383161.SubReg_TargetFlags = 0;
  D.383161.ParentMI = 0B;
  _70 = D.383161.OpKind;
  if (_70 != 5)
    goto <bb 114>;
  else
    goto <bb 118>;

...

  <bb 119>:
  D.383161 ={v} {CLOBBER};
  _436 = _69;
  _71 = _436;
  D.324881 ={v} {CLOBBER};
  _208 = MEM[(struct TrackingMDRef *)&D.324880].MD;
  if (_208 != 0B)
    goto <bb 120>;
  else
    goto <bb 121>;

...

  <bb 122>:
  _73 = _71->MI;
  _209 = _71->MF;
  D.381968.OpKind = 1;
  D.381968.SubReg_TargetFlags = 0;
  D.381968.ParentMI = 0B;
  D.381968.Contents.ImmVal = 0;
  llvm::MachineInstr::addOperand (_73, _209, &D.381968);

Note that D.324881 gets clobbered when _71 has just been assigned to point to
D.324881.  _73 and _209 then read values from this clobbered object.  That
doesn't look good.

By the time we get to .092t.cplxlower1, this appears more explicit:

  <bb 84>:
  D.383161 ={v} {CLOBBER};
  D.324881 ={v} {CLOBBER};
  _208 = MEM[(struct TrackingMDRef *)&D.324880].MD;
  if (_208 != 0B)
    goto <bb 85>;
  else
    goto <bb 86>;

...

  <bb 87>:
  _73 = D.324881.MI;
  _209 = D.324881.MF;
  D.381968.OpKind = 1;
  D.381968.SubReg_TargetFlags = 0;
  D.381968.ParentMI = 0B;
  D.381968.Contents.ImmVal = 0;
  llvm::MachineInstr::addOperand (_73, _209, &D.381968);

This leads .093t.sra to create a use-without-def situation:

  struct MachineInstr * const SR.1271;

...

  <bb 87>:
  _73 = SR.1271_292(D);
  _209 = SR.1270_58(D);
  D.381968.OpKind = 1;
  D.381968.SubReg_TargetFlags = 0;
  D.381968.ParentMI = 0B;
  D.381968.Contents.ImmVal = 0;
  llvm::MachineInstr::addOperand (_73, _209, &D.381968);

...and eventually we just initialize this thing to zero since it is
upward-exposed to the prologue, leading to the bad code gen we see.

So I assume that something bad has happened during inlining (or some other
phase where the dumps are not available).  I don't know how to investigate
further.  CCing Honza for his input...

[Bug tree-optimization/65773] [5 Regression] GCC 5.1 miscompiles LLVM function AArch64InstrInfo::loadRegFromStackSlot()

[wschmidt at gcc dot gnu.org]

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

--- Comment #6 from Bill Schmidt <wschmidt at gcc dot gnu.org> ---
$ /home/wschmidt/gcc/install/gcc-5_1/bin/g++ -O1 -std=c++11 -S
AArch64InstrInfo.ii -fno-icf       
g++: error: unrecognized command line option '-fno-icf'

[Bug tree-optimization/65773] [5 Regression] GCC 5.1 miscompiles LLVM function AArch64InstrInfo::loadRegFromStackSlot()

[trippels at gcc dot gnu.org]

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

Markus Trippelsdorf <trippels at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |trippels at gcc dot gnu.org

--- Comment #7 from Markus Trippelsdorf <trippels at gcc dot gnu.org> ---
-fno-ipa-icf

[Bug tree-optimization/65773] [5 Regression] GCC 5.1 miscompiles LLVM function AArch64InstrInfo::loadRegFromStackSlot()

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |5.0

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
comment #5 sounds like this is an issue in LLVM.  SRA (and into-SSA) now take
"advantage" of clobbers.  Try -fno-lifetime-dse (and -fstack-reuse=none)?

[Bug tree-optimization/65773] [5 Regression] GCC 5.1 miscompiles LLVM function AArch64InstrInfo::loadRegFromStackSlot()

[hubicka at gcc dot gnu.org]

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

--- Comment #9 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
Bill,
you can track inlining with -fdump-tree-einline (early inliner) and
-fdump-ipa-inline (the greedy inliner)

[Bug tree-optimization/65773] [5 Regression] GCC 5.1 miscompiles LLVM function AArch64InstrInfo::loadRegFromStackSlot()

[jakub at gcc dot gnu.org]

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

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
For better analysis, I'll note that the:
@@ -5846,13 +5848,13 @@ virtual void llvm::AArch64InstrInfo::loa
   D.391854.SubReg_TargetFlags = 0;
   D.391854.ParentMI = 0B;
   D.391854.Contents.ImmVal = 0;
-  llvm::MachineInstr::addOperand (MI_301, _440, &D.391854);
+  llvm::MachineInstr::addOperand (SR.1446_371(D), SR.1445_78(D), &D.391854);

   <bb 82>:
   D.391854 ={v} {CLOBBER};

   <bb 83>:
-  llvm::MachineInstr::addMemOperand (MI_301, _440, MMO_21);
+  llvm::MachineInstr::addMemOperand (SR.1446_371(D), SR.1445_78(D), MMO_21);

   <bb 84>:
   _79 = MEM[(struct TrackingMDRef *)&DL].MD;
change in the loadRegFromStackSlot function in *.optimized dump started with
r211725.

[Bug tree-optimization/65773] [5 Regression] GCC 5.1 miscompiles LLVM function AArch64InstrInfo::loadRegFromStackSlot()

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
So, the source in question is:
  const MachineInstrBuilder &MI = BuildMI(MBB, MBBI, DL, get(Opc))
                                      .addReg(DestReg, getDefRegState(true))
                                      .addFrameIndex(FI);
  if (Offset)  
    MI.addImm(0);
  MI.addMemOperand(MMO);
which in *.gimple dump looks like:
          try
            {
              D.382398 = &this->D.207751.D.207513.D.205713;
              D.382399 = llvm::MCInstrInfo::get (D.382398, Opc);
              D.325474 = llvm::BuildMI (MBB, MBBI, &D.325473, D.382399);
[return slot optimization]
              try
                {
                  D.382400 = llvm::getDefRegState (1);
                  D.382401 = llvm::MachineInstrBuilder::addReg (&D.325474,
DestReg, D.382400, 0);
                  MI = llvm::MachineInstrBuilder::addFrameIndex (D.382401, FI);
                }
              finally
                {
                  D.325474 = {CLOBBER};
                }
            }
          finally
            {
              llvm::DebugLoc::~DebugLoc (&D.325473);
              D.325473 = {CLOBBER};
            }
          if (Offset != 0) goto <D.382402>; else goto <D.382403>;
          <D.382402>:
          llvm::MachineInstrBuilder::addImm (MI, 0);
          goto <D.382404>;
          <D.382403>:
          <D.382404>:
          llvm::MachineInstrBuilder::addMemOperand (MI, MMO);
which suggests that the temporary that BuildMI returns goes out of scope at the
end of the
  const MachineInstrBuilder &MI = BuildMI(MBB, MBBI, DL, get(Opc))
                                      .addReg(DestReg, getDefRegState(true))
                                      .addFrameIndex(FI);
and sets the reference to the out of scope temporary.
  const
  MachineInstrBuilder &addReg(unsigned RegNo, unsigned flags = 0,
                              unsigned SubReg = 0) const {
ends with
    return *this;
and
  const MachineInstrBuilder &addFrameIndex(int Idx) const {
    MI->addOperand(*MF, MachineOperand::CreateFI(Idx));
    return *this;
  }
too.

[Bug tree-optimization/65773] [5 Regression] GCC 5.1 miscompiles LLVM function AArch64InstrInfo::loadRegFromStackSlot()

[pinskia at gcc dot gnu.org]

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

--- Comment #12 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #11)
> So, the source in question is:
>   const MachineInstrBuilder &MI = BuildMI(MBB, MBBI, DL, get(Opc))
>                                       .addReg(DestReg, getDefRegState(true))
>                                       .addFrameIndex(FI);
>  

The way to fix llvm' sources is do to:

const MachineInstrBuilder &MI = BuildMI(MBB, MBBI, DL, get(Opc));
MI.addReg(DestReg, getDefRegState(true)).addFrameIndex(FI);

Which allows for the return value of buildmi to expand its life time past the
end of the statement.

[Bug tree-optimization/65773] [5 Regression] GCC 5.1 miscompiles LLVM function AArch64InstrInfo::loadRegFromStackSlot()

[james.molloy at arm dot com]

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

James Molloy <james.molloy at arm dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |james.molloy at arm dot com

--- Comment #13 from James Molloy <james.molloy at arm dot com> ---
Hi,

This has just been pinged at me. Thanks for debugging this and sorry about the
broken code.

Just a note that the actual problem/fix is even more simple. The problem is
we're asking for a reference instead of a copy (which is the pattern used
elsewhere), so simply removing the '&' will enforce correct behaviour.

Cheers,

James

[Bug tree-optimization/65773] [5 Regression] GCC 5.1 miscompiles LLVM function AArch64InstrInfo::loadRegFromStackSlot()

[james.molloy at arm dot com]

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

--- Comment #14 from James Molloy <james.molloy at arm dot com> ---
Hi,

For completeness, I just fixed this in LLVM r235088
(http://reviews.llvm.org/rL235088).

Cheers,

James