[llvm] r224009 - Bugfix in InlineSpiller::traceSiblingValue().

[Jonas Paulsson]

Hello again,

Here is also a patch for InlineSpiller.cpp with an assert that triggers on this problem, in case anyone wants to try. Sorry for repeated mailings.

/Jonas

From: Jonas Paulsson
Sent: den 12 december 2014 12:26
To: 'Quentin Colombet'; David Blaikie
Cc: Patrik Hägglund H; llvm-commits at cs.uiuc.edu
Subject: RE: [llvm] r224009 - Bugfix in InlineSpiller::traceSiblingValue().

Hi,

I tried to reproduce the error on arm and x86 targets and came pretty close at least with arm by using inline asm as you suggested. But, unfortunately I could not reproduce the exact conditions where the LiveInterval would get split in the same way before the inner loops.

I attach the c file I was using in case someone else might want to have a go. To get an idea on what the exact problem was, see the conversations between me and Quentin on the mailing list.

/Jonas



From: Quentin Colombet [mailto:qcolombet at apple.com]
Sent: den 11 december 2014 20:20
To: David Blaikie
Cc: Patrik Hägglund H; llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>; Jonas Paulsson
Subject: Re: [llvm] r224009 - Bugfix in InlineSpiller::traceSiblingValue().

Hi David,

Indeed, this has been brought up during the review.

Jonas hasn't been able to find a test case for in-tree targets so far.
I've given him a few hints that may help him to reproduce the problem with in-tree targets, but the conditions to make that happen are quite difficult to have all lined up (that's why it never occurred so far, whereas the bug was there forever).

Jonas may have additional comments.

Thanks,
-Quentin

On Dec 11, 2014, at 9:58 AM, David Blaikie <dblaikie at gmail.com<mailto:dblaikie at gmail.com>> wrote:

Any chance of a test case? (I assume this was brought up in code review - but it's helpful to have the justification included in the commit message, ideally)

On Thu, Dec 11, 2014 at 2:40 AM, Patrik Hagglund <patrik.h.hagglund at ericsson.com<mailto:patrik.h.hagglund at ericsson.com>> wrote:
Author: patha
Date: Thu Dec 11 04:40:17 2014
New Revision: 224009

URL: http://llvm.org/viewvc/llvm-project?rev=224009&view=rev
Log:
Bugfix in InlineSpiller::traceSiblingValue().

Properly determine whether or not a phi was added by splitting.
Check against the current VNInfo of OrigLI instead of against the
OrigVNI argument.

Patch provided by Jonas Paulsson. Reviewed by Quentin Colombet.

Modified:
    llvm/trunk/lib/CodeGen/InlineSpiller.cpp

Modified: llvm/trunk/lib/CodeGen/InlineSpiller.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/InlineSpiller.cpp?rev=224009&r1=224008&r2=224009&view=diff
==============================================================================
--- llvm/trunk/lib/CodeGen/InlineSpiller.cpp (original)
+++ llvm/trunk/lib/CodeGen/InlineSpiller.cpp Thu Dec 11 04:40:17 2014
@@ -508,6 +508,7 @@ MachineInstr *InlineSpiller::traceSiblin
   SmallVector<std::pair<unsigned, VNInfo*>, 8> WorkList;
   WorkList.push_back(std::make_pair(UseReg, UseVNI));

+  LiveInterval &OrigLI = LIS.getInterval(Original);
   do {
     unsigned Reg;
     VNInfo *VNI;
@@ -521,8 +522,11 @@ MachineInstr *InlineSpiller::traceSiblin

     // Trace through PHI-defs created by live range splitting.
     if (VNI->isPHIDef()) {
-      // Stop at original PHIs.  We don't know the value at the predecessors.
-      if (VNI->def == OrigVNI->def) {
+      // Stop at original PHIs.  We don't know the value at the
+      // predecessors. Look up the VNInfo for the current definition
+      // in OrigLI, to properly determine whether or not this phi was
+      // added by splitting.
+      if (VNI->def == OrigLI.getVNInfoAt(VNI->def)->def) {
         DEBUG(dbgs() << "orig phi value\n");
         SVI->second.DefByOrigPHI = true;
         SVI->second.AllDefsAreReloads = false;
@@ -542,7 +546,6 @@ MachineInstr *InlineSpiller::traceSiblin
       // Separate all values dominated by OrigVNI into PHIs and non-PHIs.
       SmallVector<VNInfo*, 8> PHIs, NonPHIs;
       LiveInterval &LI = LIS.getInterval(Reg);
-      LiveInterval &OrigLI = LIS.getInterval(Original);

       for (LiveInterval::vni_iterator VI = LI.vni_begin(), VE = LI.vni_end();
            VI != VE; ++VI) {


_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141212/ce013d49/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-InlineSpiller-assert.patch
Type: application/octet-stream
Size: 1748 bytes
Desc: 0001-InlineSpiller-assert.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141212/ce013d49/attachment.obj>