[PATCH] D14454: [WinEH] Fix mutli-parent cloning

[Kaylor, Andrew via llvm-commits]

Thanks, Yaron.

This change looks good to me.  I didn’t get to it earlier, and I don’t want to commit it now in case it this late in the day, but I’ll commit this first thing tomorrow.

-Andy


From: Yaron Keren [mailto:yaron.keren at gmail.com]
Sent: Wednesday, November 11, 2015 9:03 AM
To: reviews+D14454+public+ed63114aca7e347a at reviews.llvm.org; Phabricator <reviews at reviews.llvm.org>
Cc: Kaylor, Andrew <andrew.kaylor at intel.com>; Reid Kleckner <rnk at google.com>; David Majnemer <david.majnemer at gmail.com>; jotrem at microsoft.com; llvm-commits <llvm-commits at lists.llvm.org>
Subject: Re: [PATCH] D14454: [WinEH] Fix mutli-parent cloning

Hi Andy,

The change from std::set to SetVector gains stable iteration order but loses stable iterators...
Specifically, it invalidates BlockColors[BB].end() after every remove(), so it should not be cached in the End variable. Even if we solve this by testing It != BlockColors[BB].end(), after removing the last item 'It' will be at the (new after removal) end()+1 and then compared to the (new after removal) end() which is probably undefined behaviour for an iterator. Finally, erasing items in a loop in a SetVector is quadratic complexity.

To solve all at once, we could split the loop into two steps:

        for (BasicBlock *ContainingFunclet : BlockColors[BB])
          if (ContainingFunclet != CorrectColor)
            FuncletBlocks[ContainingFunclet].erase(BB);
        BlockColors[BB].remove_if([&](BasicBlock *ContainingFunclet) {
          return ContainingFunclet != CorrectColor;
        });

Please feel free to modify the attached patch and commit,

Yaron




-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151112/e4e956e5/attachment-0001.html>