[LLVMugs][Bug 20122] Patch ready
Hal Finkel
hfinkel at anl.gov
Thu Jul 3 07:57:28 PDT 2014
----- Original Message -----
> From: "wuhui1973" <wuhui1973 at 163.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: llvm-commits at cs.uiuc.edu
> Sent: Thursday, July 3, 2014 4:42:05 AM
> Subject: Re: [LLVMugs][Bug 20122] Patch ready
>
>
>
> 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.
I'll look at it, thanks.
One thing you'll need to do is add some kind of (void)limit; statement so that limit is not unused in a release build (which will produce a warning).
-Hal
>
>
> 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
>
>
>
>
--
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory
More information about the llvm-commits
mailing list