[PATCH] fix for PR16393: miscompile with struct byval

[Manman Ren]

On Jul 12, 2013, at 1:54 PM, Jim Grosbach wrote:

> Hi Manman,
> 
> This looks really good and I’m happy with the general approach. One minor note is that I don’t think you need to use a DenseMap<> between the SPAdj values and the MBB pointers. Instead you can just allocate a SmallVector<unsigned> and index it via MachineBasicBlock::Number. MachineFunction::getNumBlockIDs() will give you how many entries to allocate into the vector (it’s >= the number of MBBs since the numbering may have gaps if an MBB was deleted).

Hi Jim,

Thanks for reviewing the patch.
Updated patch to use SmallVector instead of DenseMap:
--- lib/CodeGen/PrologEpilogInserter.cpp	(revision 186215)
+++ lib/CodeGen/PrologEpilogInserter.cpp	(working copy)
@@ -728,7 +728,33 @@
 void PEI::replaceFrameIndices(MachineFunction &Fn) {
   if (!Fn.getFrameInfo()->hasStackObjects()) return; // Nothing to do?
 
+  // Store SPAdj at exit of a basic block.
+  SmallVector<int, 8> SPState;
+  SPState.resize(Fn.getNumBlockIDs());
+  SmallPtrSet<MachineBasicBlock*, 8> Reachable;
+
+  // Iterate over the reachable blocks in DFS order.
+  for (df_ext_iterator<MachineFunction*, SmallPtrSet<MachineBasicBlock*, 8> >
+       DFI = df_ext_begin(&Fn, Reachable), DFE = df_ext_end(&Fn, Reachable);
+       DFI != DFE; ++DFI) {
+    int SPAdj = 0;
+    // Check the exit state of the DFS stack predecessor.
+    if (DFI.getPathLength() >= 2) {
+      MachineBasicBlock *StackPred = DFI.getPath(DFI.getPathLength() - 2);
+      assert(Reachable.count(StackPred) &&
+             "DFS stack predecessor is already visited.\n");
+      SPAdj = SPState[StackPred->getNumber()];
+    }
+    MachineBasicBlock *BB = *DFI;
+    replaceFrameIndices(BB, Fn, SPAdj);
+    SPState[BB->getNumber()] = SPAdj;
+  }
+
+  // Handle the unreachable blocks.
   for (MachineFunction::iterator BB = Fn.begin(), E = Fn.end(); BB != E; ++BB) {
+    if (Reachable.count(BB))
+      // Already handled in DFS traversal.
+      continue;
     int SPAdj = 0;
     replaceFrameIndices(BB, Fn, SPAdj);
   }

Manman

> 
> -Jim
> 
> On Jul 11, 2013, at 9:15 PM, Manman Ren <mren at apple.com> wrote:
> 
>> 
>> On Jul 11, 2013, at 6:40 PM, Jakob Stoklund Olesen wrote:
>> 
>>> 
>>> On Jul 11, 2013, at 12:07 PM, Manman Ren <mren at apple.com> wrote:
>>> 
>>>> Modify MachineVerifier to make sure
>>>> 1> on every path through the CFG, a FrameSetup <n> is always followed by a
>>>>   FrameDestroy <n> and a FrameDestroy is always followed by a FrameSetup.
>>>> 2> stack adjustments are identical on all CFG edges to a merge point.
>>> 
>>> One more thing: You need to verify that the frame is destroyed at the end of a return block.
>> Good point, I can add this extra check.
>> 
>> Manman
>> 
>>> 
>>> /jakob
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130712/a672deee/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: spadj_pei.patch
Type: application/octet-stream
Size: 2934 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130712/a672deee/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130712/a672deee/attachment-0001.html>