[LLVMugs][Bug 20122] Patch ready

wuhui1973 wuhui1973 at 163.com
Wed Jul 2 01:44:42 PDT 2014



Hello Hal:


I have caught the debug log, please see attachment. Below is the finding:


r600 has a large register file, so the generated Unitset is large too.
> Before union, there are 26 sets
> After union, there are 59 sets
> After prune, the number goes from 59 to 55 (means the sets are quite diversity)


The original alogrithm is effective, I enhence the debug log a little to output the sets filtered out, can see lots of them had been filtered.


Because the operation is find the transtive closure union of any two overlapping sets, the original operating sets are roughly 26*26 for this example.
So I think we should set the limit out of this number. For example, we may except in these combinations only around 10% will be kept, so using 26*26/10 
instead of 2*26. Please refer the patch for the solution.


Regards & Thanks







At 2014-07-01 07:21:45, "Hal Finkel" <hfinkel at anl.gov> wrote:
>----- Original Message -----
>> From: "wuhui1973" <wuhui1973 at 163.com>
>> To: "Hal Finkel" <hfinkel at anl.gov>
>> Cc: llvm-commits at cs.uiuc.edu
>> Sent: Tuesday, July 1, 2014 4:57:06 AM
>> Subject: Re:Re: [LLVMugs][Bug 20122] Patch ready
>> 
>> 
>> 
>> 
>> Hello Hal:
>> 
>> 
>> I found this comment about the assertion:
>> 
>> 
>> 
>> // In theory, this is combinatorial. In practice, it needs to be
>> bounded
>> // by a small number of sets for regpressure to be efficient.
>> // If the assert is hit, we need to implement pruning.
>> assert(Idx < (2*NumRegUnitSubSets) && "runaway unit set inference");
>> 
>> Seems the author had forseen this problem, and he (she) suggested to
>> prune the unitset.
>> I try to prune the unitset by pruneUnitSets if it exceeds the limit,
>> but not succeed - the set is not reduced.
>> 
>> 
>> I am now trying to open the debug output available in the code and
>> see what's happening.
>> Just a question: How to enable DEBUG macro in this source, which is
>> run by tablegen?
>
>If you run with the -debug command-line argument you should get the debugging output.
>
>Thanks again,
>Hal
>
>> 
>> 
>> However, this error can be avoid, just using assert(Idx <
>> (3*NumRegUnitSubSets) && "runaway unit set inference");
>> 
>> 
>> Regards & Thanks At 2014-07-01 05:42:20, "Hal Finkel" <
>> hfinkel at anl.gov > wrote:
>> >Hi,
>> >
>> >With this patch applied, building LLVM's AMDGPU target fails:
>> >
>> >/src/llvm-trunk/utils/TableGen/CodeGenRegisters.cpp:1588: void
>> >llvm::CodeGenRegBank::computeRegUnitSets(): Assertion `Idx <
>> >(2*NumRegUnitSubSets) && "runaway unit set inference"' failed.
>> >
>> >Maybe the threshold in the assert just needs to be loosened, but
>> >we'd need to understand what is going on here.
>> >
>> > -Hal
>> >
>> >----- Original Message -----
>> >> From: "wuhui1973" < wuhui1973 at 163.com >
>> >> To: llvm-commits at cs.uiuc.edu >> Sent: Sunday, June 29, 2014
>> >> 11:08:40 PM
>> >> Subject: [LLVMugs][Bug 20122] Patch ready
>> >> 
>> >> 
>> >> 
>> >> 
>> >> 
>> >> [ reply ] [-] Description hui wu 2014-06-24 21:35:52 CDT In
>> >> function
>> >> CodeGenRegBank::computeRegUnitSets, below code fragment is used to
>> >> calculate the transitive closure of set union of intersecting
>> >> sets.
>> >> 
>> >>   // Iterate over all unit sets, including new ones added by this
>> >>   loop.
>> >>   unsigned NumRegUnitSubSets = RegUnitSets.size();
>> >>   for (unsigned Idx = 0, EndIdx = RegUnitSets.size(); Idx !=
>> >>   EndIdx;
>> >>   ++Idx) {
>> >>     // In theory, this is combinatorial. In practice, it needs to
>> >>     be
>> >>     bounded
>> >>     // by a small number of sets for regpressure to be efficient.
>> >>     // If the assert is hit, we need to implement pruning.
>> >>     assert(Idx < (2*NumRegUnitSubSets) && "runaway unit set
>> >>     inference");
>> >> 
>> >>     // Compare new sets with all original classes.
>> >>     for (unsigned SearchIdx = (Idx >= NumRegUnitSubSets) ? 0 :
>> >>     Idx+1;
>> >>          SearchIdx != EndIdx; ++SearchIdx) {
>> >>       std::set<unsigned> Intersection;
>> >>       std::set_intersection(RegUnitSets[Idx].Units.begin(),
>> >>                             RegUnitSets[Idx].Units.end(),
>> >>                             RegUnitSets[SearchIdx].Units.begin(),
>> >>                             RegUnitSets[SearchIdx].Units.end(),
>> >>                             std::inserter(Intersection,
>> >>                             Intersection.begin()));
>> >>       if (Intersection.empty())
>> >>         continue;
>> >> 
>> >>       // Speculatively grow the RegUnitSets to hold the new set.
>> >>       RegUnitSets.resize(RegUnitSets.size() + 1);
>> >>       RegUnitSets.back().Name =
>> >>         RegUnitSets[Idx].Name + "+" + RegUnitSets[SearchIdx].Name;
>> >> 
>> >>       std::set_union(RegUnitSets[Idx].Units.begin(),
>> >>                      RegUnitSets[Idx].Units.end(),
>> >>                      RegUnitSets[SearchIdx].Units.begin(),
>> >>                      RegUnitSets[SearchIdx].Units.end(),
>> >>                      std::inserter(RegUnitSets.back().Units,
>> >>                                    RegUnitSets.back().Units.begin()));
>> >> 
>> >>       // Find an existing RegUnitSet, or add the union to the
>> >>       unique
>> >>       sets.
>> >>       std::vector<RegUnitSet>::const_iterator SetI =
>> >>         findRegUnitSet(RegUnitSets, RegUnitSets.back());
>> >>       if (SetI != llvm::prior(RegUnitSets.end()))
>> >>         RegUnitSets.pop_back();
>> >>     }
>> >>   }
>> >> 
>> >> However, the use of EndIdx like that is not correct, it makes the
>> >> for
>> >> loop exiting too early without handling the new added sets. The
>> >> correct way should be:
>> >> 
>> >>   // Iterate over all unit sets, including new ones added by this
>> >>   loop.
>> >>   unsigned NumRegUnitSubSets = RegUnitSets.size();
>> >>   for (unsigned Idx = 0; Idx != RegUnitSets.size(); ++Idx) {
>> >>     // In theory, this is combinatorial. In practice, it needs to
>> >>     be
>> >>     bounded
>> >>     // by a small number of sets for regpressure to be efficient.
>> >>     // If the assert is hit, we need to implement pruning.
>> >>     assert(Idx < (2*NumRegUnitSubSets) && "runaway unit set
>> >>     inference");
>> >> 
>> >>     // Compare new sets with all original classes.
>> >>     for (unsigned SearchIdx = (Idx >= NumRegUnitSubSets) ? 0 :
>> >>     Idx+1;
>> >>          SearchIdx != RegUnitSets.size(); ++SearchIdx) {
>> >>       std::set<unsigned> Intersection;
>> >>       std::set_intersection(RegUnitSets[Idx].Units.begin(),
>> >>                             RegUnitSets[Idx].Units.end(),
>> >>                             RegUnitSets[SearchIdx].Units.begin(),
>> >>                             RegUnitSets[SearchIdx].Units.end(),
>> >>                             std::inserter(Intersection,
>> >>                             Intersection.begin()));
>> >>       if (Intersection.empty())
>> >>         continue;
>> >> 
>> >>       // Speculatively grow the RegUnitSets to hold the new set.
>> >>       RegUnitSets.resize(RegUnitSets.size() + 1);
>> >>       RegUnitSets.back().Name =
>> >>         RegUnitSets[Idx].Name + "+" + RegUnitSets[SearchIdx].Name;
>> >> 
>> >>       std::set_union(RegUnitSets[Idx].Units.begin(),
>> >>                      RegUnitSets[Idx].Units.end(),
>> >>                      RegUnitSets[SearchIdx].Units.begin(),
>> >>                      RegUnitSets[SearchIdx].Units.end(),
>> >>                      std::inserter(RegUnitSets.back().Units,
>> >>                                    RegUnitSets.back().Units.begin()));
>> >> 
>> >>       // Find an existing RegUnitSet, or add the union to the
>> >>       unique
>> >>       sets.
>> >>       std::vector<RegUnitSet>::const_iterator SetI =
>> >>         findRegUnitSet(RegUnitSets, RegUnitSets.back());
>> >>       if (SetI != llvm::prior(RegUnitSets.end()))
>> >>         RegUnitSets.pop_ back();
>> >>     }
>> >>   }
>> >> 
>> >> [ reply ] [-] Comment 1 Hal Finkel 2014-06-25 05:02:27 CDT Thanks
>> >> for
>> >> filing this bug; do you know if this is still a problem in llvm
>> >> trunk? If so, the best way to provide this fix is to send a patch
>> >> to
>> >> the llvm-commits mailing list. That is how we normally review
>> >> proposed patches.
>> >> http://llvm.org/docs/DeveloperPolicy.html#making-and-submitting-a-patch
>> >> Also, if you know how to generate a test that triggers the
>> >> incorrect
>> >> behavior, that would be great!
>> >> 
>> >> 
>> >> _______________________________________________
>> >> llvm-commits mailing list
>> >> llvm-commits at cs.uiuc.edu >>
>> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >> 
>> >
>> >--
>> >Hal Finkel
>> >Assistant Computational Scientist
>> >Leadership Computing Facility
>> >Argonne National Laboratory
>> 
>> 
>
>-- 
>Hal Finkel
>Assistant Computational Scientist
>Leadership Computing Facility
>Argonne National Laboratory
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140702/90c0cbe7/attachment.html>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: bug_20122.log
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140702/90c0cbe7/attachment.ksh>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bug_20122.patch
Type: application/octet-stream
Size: 2137 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140702/90c0cbe7/attachment.obj>


More information about the llvm-commits mailing list