[LLVMugs][Bug 20122] Patch ready

[Hal Finkel]

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