[PATCH] fix for PR16393: miscompile with struct byval

[Manman Ren]

Updated verifier patch to
1> use a SmallVector instead of a DenseMap
2> verify at end of a return block, the frame is destroyed.

Thanks,
Manman

On Jul 12, 2013, at 4:29 PM, Manman Ren wrote:

> 
> 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
> 
> <spadj_pei.patch>
>> 
>> -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
>> 
> 
> _______________________________________________
> llvm-commits mailing list
> 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/20130715/f4f581b3/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: spadj_verify.patch
Type: application/octet-stream
Size: 6671 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130715/f4f581b3/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130715/f4f581b3/attachment-0001.html>