<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#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="moz-txt-link-freetext" href="https://llvm.org/docs/FuzzingLLVM.html">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="moz-cite-prefix">On 09/12/2018 07:28 AM, Alexandros
Lamprineas via llvm-commits wrote:<br>
</div>
<blockquote type="cite"
cite="mid:20180912142823.36CE581DC3@lists.llvm.org">
<pre wrap="">Author: alelab01
Date: Wed Sep 12 07:28:23 2018
New Revision: 342055
URL: <a class="moz-txt-link-freetext" href="http://llvm.org/viewvc/llvm-project?rev=342055&view=rev">http://llvm.org/viewvc/llvm-project?rev=342055&view=rev</a>
Log:
[GVNHoist] computeInsertionPoints() miscalculates IDF
Fix for <a class="moz-txt-link-freetext" href="https://bugs.llvm.org/show_bug.cgi?id=38912">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="moz-txt-link-freetext" href="https://reviews.llvm.org/D51980">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="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">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="moz-txt-link-abbreviated" href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>
<a class="moz-txt-link-freetext" href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote>
<br>
</body>
</html>