[LLVMugs][Bug 20122] Patch ready

[wuhui1973]

|
[reply] [-]Descriptionhui 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 1Hal 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!
|
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140630/ff9d3d8a/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Bug_20122.patch
Type: application/octet-stream
Size: 1085 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140630/ff9d3d8a/attachment.obj>