[llvm] r342055 - [GVNHoist] computeInsertionPoints() miscalculates IDF
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 17 11:26:53 PDT 2018
Alexandros,
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.
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.
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.
Have you run their test suites? More than once? (i.e. looking for
miscompiles which introduce non-deterministic failures)
Philip
On 09/17/2018 04:05 AM, Alexandros Lamprineas wrote:
>
> Hi Philip,
>
>
> 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.
>
>
> 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 GVNHoist so far. A bootstrap build of clang with UbSan
> enabled seems to be the most intense test for GVNHoist. It has
> revealed interesting bugs that have been already addressed. On that
> note, I'll try to re-enable it again today.
>
>
> Regards,
>
> Alexandros
>
>
>
> ------------------------------------------------------------------------
> *From:* Philip Reames <listmail at philipreames.com>
> *Sent:* Thursday, September 13, 2018 1:06 AM
> *To:* Alexandros Lamprineas; llvm-commits at lists.llvm.org
> *Subject:* Re: [llvm] r342055 - [GVNHoist] computeInsertionPoints()
> miscalculates IDF
>
> 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.
>
> However, I would like to suggest a change in approach from the current
> "enable and see what breaks" approach.
>
> 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?
>
> Specifically, we have the following tooling:
>
> * csmith is available and I believe regularly run.
> * We have the llvm-opt-fuzzer infrastructure which is run
> continuously on OSSFuzz. Adapting this to exercise GVNHoist
> should be straight-forward. (see
> https://llvm.org/docs/FuzzingLLVM.html
> <https://llvm.org/docs/FuzzingLLVM.html>)
> * 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.
>
> 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.
>
> Philip
>
>
> On 09/12/2018 07:28 AM, Alexandros Lamprineas via llvm-commits wrote:
>> Author: alelab01
>> Date: Wed Sep 12 07:28:23 2018
>> New Revision: 342055
>>
>> URL:http://llvm.org/viewvc/llvm-project?rev=342055&view=rev
>> Log:
>> [GVNHoist] computeInsertionPoints() miscalculates IDF
>>
>> Fix forhttps://bugs.llvm.org/show_bug.cgi?id=38912.
>>
>> 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:https://reviews.llvm.org/D51980
>>
>> Modified:
>> llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp
>>
>> Modified: llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp
>> URL:http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp?rev=342055&r1=342054&r2=342055&view=diff
>> ==============================================================================
>> --- 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
>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180917/b441c2fd/attachment.html>
More information about the llvm-commits
mailing list