<html>
<head>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Alexandros,</p>
<p><br>
</p>
<p>Thanks for sharing your testing approach and including that in
the recommit message. Overall, your approach seems completely
reasonable, so let's discuss details. Since tone is often lost in
email, let me be clear that I'm aiming for friendly brainstorming,
nothing more.<br>
</p>
<p><br>
</p>
<p>When you say "for a couple of days" how many resources were
devoted? My experience is that you need 1000s of CPU hours to get
anything useful out of a fuzzer. This is one of the strengths of
running things through OSSFuzz. <br>
</p>
<p><br>
</p>
<p>Have you built any other large pieces of software with GVNHoist
enabled? If so, which ones? And with which build
configurations? (ubsan for instance?) This is a good approach
for smoking out crashes and hangs. Another good approach is to
bugpoint each previous failure. (On a failing build.) Bugpoint
makes a very effective mutation fuzzer in practice. <br>
</p>
<p><br>
</p>
<p>Have you run their test suites? More than once? (i.e. looking
for miscompiles which introduce non-deterministic failures)<br>
</p>
<p><br>
</p>
<p>Philip<br>
</p>
<br>
<div class="moz-cite-prefix">On 09/17/2018 04:05 AM, Alexandros
Lamprineas wrote:<br>
</div>
<blockquote type="cite"
cite="mid:HE1PR0802MB24274B0E0ED21F7FC7CAB773E21E0@HE1PR0802MB2427.eurprd08.prod.outlook.com">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<style type="text/css" style="display:none;"><!-- P {margin-top:0;margin-bottom:0;} --></style>
<div id="divtagdefaultwrapper" style="font-size: 12pt; color:
rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif,
EmojiFont, "Apple Color Emoji", "Segoe UI
Emoji", NotoColorEmoji, "Segoe UI Symbol",
"Android Emoji", EmojiSymbols;" dir="ltr">
<p style="margin-top:0;margin-bottom:0">Hi Philip,</p>
<p style="margin-top:0;margin-bottom:0"><br>
</p>
<p style="margin-top:0;margin-bottom:0">Thanks for your
suggestions. I indeed performed as many proactive checks as I
could every time I re-enabled GVNHoist, but
unfortunately buildbots are always configured slightly
different from our local environment.</p>
<p style="margin-top:0;margin-bottom:0"><br>
</p>
<p style="margin-top:0;margin-bottom:0">I've been running both
csmith and llvm-opt-fuzzer overnight for a couple of days
already, but none of them has caught anything interesting for
<span style="font-family: Calibri, Helvetica, sans-serif,
EmojiFont, "Apple Color Emoji", "Segoe UI
Emoji", NotoColorEmoji, "Segoe UI Symbol",
"Android Emoji", EmojiSymbols; font-size: 16px;">
GVNHoist so far</span>. A bootstrap build of clang with
UbSan enabled seems to be the most intense test for
<span style="font-family: Calibri, Helvetica, sans-serif,
EmojiFont, "Apple Color Emoji", "Segoe UI
Emoji", NotoColorEmoji, "Segoe UI Symbol",
"Android Emoji", EmojiSymbols; font-size: 16px;">
GVNHoist</span>. It has revealed interesting bugs that have
been already addressed. On that note, I'll try to re-enable it
again today.</p>
<p style="margin-top:0;margin-bottom:0"><br>
</p>
<p style="margin-top:0;margin-bottom:0">Regards,</p>
<p style="margin-top:0;margin-bottom:0">Alexandros</p>
<br>
<br>
<div style="color: rgb(0, 0, 0);">
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font style="font-size:11pt"
face="Calibri, sans-serif" color="#000000"><b>From:</b>
Philip Reames <a class="moz-txt-link-rfc2396E" href="mailto:listmail@philipreames.com"><listmail@philipreames.com></a><br>
<b>Sent:</b> Thursday, September 13, 2018 1:06 AM<br>
<b>To:</b> Alexandros Lamprineas;
<a class="moz-txt-link-abbreviated" href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<b>Subject:</b> Re: [llvm] r342055 - [GVNHoist]
computeInsertionPoints() miscalculates IDF</font>
<div> </div>
</div>
<meta content="text/html; charset=utf-8">
<div style="background-color:#FFFFFF">
<p>I've noticed another round of work recently on fixing
issues in GVNHoist. First, let me start by saying, thank
you for being responsive to issues and disabling quickly
while investigation is in progress. That makes this much
less disruptive for the rest of the community.</p>
<p>However, I would like to suggest a change in approach
from the current "enable and see what breaks" approach.
<br>
</p>
<p>We have multiple community fuzzing efforts currently
active. Many of the issues being found seem like things
that fuzzers would be good at uncovering. Can I request
that you make a proactive effort to leverage testing
infrastructure such a fuzzers before re-enabling GVNHoist
by default?</p>
<p>Specifically, we have the following tooling:</p>
<ul>
<li>csmith is available and I believe regularly run. <br>
</li>
<li>We have the llvm-opt-fuzzer infrastructure which is
run continuously on OSSFuzz. Adapting this to exercise
GVNHoist should be straight-forward. (see
<a class="x_moz-txt-link-freetext"
href="https://llvm.org/docs/FuzzingLLVM.html"
moz-do-not-send="true">
https://llvm.org/docs/FuzzingLLVM.html</a>)<br>
</li>
<li>We (Azul) have a Java fuzzer which has found several
subtle issues in GVN itself. We can run a few cycles
for you at some point, but since exposing the failing
test cases is very manual for us, I'd ask you explore
the other options first.
<br>
</li>
</ul>
<p>An alternate approach would be to identify one or more
large projects - say Chromium, Clang itself -, build with
GVN hoist enabled, and debug any failures which result.
Lest this is not misread, let me be quick to say that I
haven't followed this effort closely. It's entirely
possible this has already been done and this is simply not
visible to me as a casual observer. If so, it might be
helpful to spell out what has already been done and link
to it from future re-enable commits.</p>
<p>Philip<br>
</p>
<br>
<div class="x_moz-cite-prefix">On 09/12/2018 07:28 AM,
Alexandros Lamprineas via llvm-commits wrote:<br>
</div>
<blockquote type="cite">
<pre>Author: alelab01
Date: Wed Sep 12 07:28:23 2018
New Revision: 342055
URL: <a class="x_moz-txt-link-freetext" href="http://llvm.org/viewvc/llvm-project?rev=342055&view=rev" moz-do-not-send="true">http://llvm.org/viewvc/llvm-project?rev=342055&view=rev</a>
Log:
[GVNHoist] computeInsertionPoints() miscalculates IDF
Fix for <a class="x_moz-txt-link-freetext" href="https://bugs.llvm.org/show_bug.cgi?id=38912" moz-do-not-send="true">https://bugs.llvm.org/show_bug.cgi?id=38912</a>.
In GVNHoist::computeInsertionPoints() we iterate over the Value
Numbers and calculate the Iterated Dominance Frontiers without
clearing the IDFBlocks vector first. IDFBlocks ends up accumulating
an insane number of basic blocks, which bloats the compilation time
of SemaChecking.cpp with ubsan enabled.
Differential Revision: <a class="x_moz-txt-link-freetext" href="https://reviews.llvm.org/D51980" moz-do-not-send="true">https://reviews.llvm.org/D51980</a>
Modified:
llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp
Modified: llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp
URL: <a class="x_moz-txt-link-freetext" href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp?rev=342055&r1=342054&r2=342055&view=diff" moz-do-not-send="true">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp?rev=342055&r1=342054&r2=342055&view=diff</a>
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp Wed Sep 12 07:28:23 2018
@@ -155,7 +155,6 @@ struct CHIArg {
using CHIIt = SmallVectorImpl<CHIArg>::iterator;
using CHIArgs = iterator_range<CHIIt>;
-using CHICache = DenseMap<BasicBlock *, SmallPtrSet<Instruction *, 4>>;
using OutValuesType = DenseMap<BasicBlock *, SmallVector<CHIArg, 2>>;
using InValuesType =
DenseMap<BasicBlock *, SmallVector<std::pair<VNType, Instruction *>, 2>>;
@@ -767,7 +766,6 @@ private:
ReverseIDFCalculator IDFs(*PDT);
OutValuesType OutValue;
InValuesType InValue;
- CHICache CachedCHIs;
for (const auto &R : Ranks) {
const SmallVecInsn &V = Map.lookup(R);
if (V.size() < 2)
@@ -786,6 +784,7 @@ private:
// which currently have dead terminators that are control
// dependence sources of a block which is in NewLiveBlocks.
IDFs.setDefiningBlocks(VNBlocks);
+ IDFBlocks.clear();
IDFs.calculate(IDFBlocks);
// Make a map of BB vs instructions to be hoisted.
@@ -798,8 +797,7 @@ private:
for (unsigned i = 0; i < V.size(); ++i) {
CHIArg C = {VN, nullptr, nullptr};
// Ignore spurious PDFs.
- if (DT->properlyDominates(IDFB, V[i]->getParent()) &&
- CachedCHIs[IDFB].insert(V[i]).second) {
+ if (DT->properlyDominates(IDFB, V[i]->getParent())) {
OutValue[IDFB].push_back(C);
LLVM_DEBUG(dbgs() << "\nInsertion a CHI for BB: " << IDFB->getName()
<< ", for Insn: " << *V[i]);
_______________________________________________
llvm-commits mailing list
<a class="x_moz-txt-link-abbreviated" href="mailto:llvm-commits@lists.llvm.org" moz-do-not-send="true">llvm-commits@lists.llvm.org</a>
<a class="x_moz-txt-link-freetext" href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" moz-do-not-send="true">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote>
<br>
</div>
</div>
</div>
IMPORTANT NOTICE: The contents of this email and any attachments
are confidential and may also be privileged. If you are not the
intended recipient, please notify the sender immediately and do
not disclose the contents to any other person, use it for any
purpose, or store or copy the information in any medium. Thank
you.
</blockquote>
<br>
</body>
</html>