[llvm-commits] [llvm] r156804 - /llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

[Duncan Sands]

Hi Stepan, while I have no idea what this code is doing (hopefully Anton does) I
do have one comment:

> SelectionDAGBuilder::Clusterify : main functinality was replaced with CRSBuilder::optimize, so big part of Clusterify's code was reduced.

> --- llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp (original)
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp Tue May 15 00:09:41 2012
> @@ -51,6 +51,7 @@
>   #include "llvm/Target/TargetLowering.h"
>   #include "llvm/Target/TargetOptions.h"
>   #include "llvm/Support/CommandLine.h"
> +#include "llvm/Support/CRSBuilder.h"
>   #include "llvm/Support/Debug.h"
>   #include "llvm/Support/ErrorHandling.h"
>   #include "llvm/Support/MathExtras.h"
> @@ -2038,7 +2039,7 @@
>
>   static APInt ComputeRange(const APInt&First, const APInt&Last) {
>     uint32_t BitWidth = std::max(Last.getBitWidth(), First.getBitWidth()) + 1;
> -  APInt LastExt = Last.sext(BitWidth), FirstExt = First.sext(BitWidth);
> +  APInt LastExt = Last.zext(BitWidth), FirstExt = First.zext(BitWidth);
>     return (LastExt - FirstExt + 1ULL);
>   }
>
> @@ -2104,7 +2105,7 @@
>       const APInt&Low = cast<ConstantInt>(I->Low)->getValue();
>       const APInt&High = cast<ConstantInt>(I->High)->getValue();
>
> -    if (Low.sle(TEI)&&  TEI.sle(High)) {
> +    if (Low.ule(TEI)&&  TEI.ule(High)) {

I think both of these may be the wrong way to test whether TEI is between Low
and High, instead it should be: (TEI - Low).ule(High - Low).  This works whether
or not Low and High are considered to be signed or unsigned.  However it won't
work properly if (for example) High.slt(Low) indicates an empty range rather
than a very wide range that wrapped around.

Ciao, Duncan.

PS: This patch caused miscompilation as shown by several dragonegg buildbots.
PPS: Did anyone review this patch?

>         DestBBs.push_back(I->BB);
>         if (TEI==High)
>           ++I;
> @@ -2183,8 +2184,10 @@
>       const APInt&LEnd = cast<ConstantInt>(I->High)->getValue();
>       const APInt&RBegin = cast<ConstantInt>(J->Low)->getValue();
>       APInt Range = ComputeRange(LEnd, RBegin);
> -    assert((Range - 2ULL).isNonNegative()&&
> -           "Invalid case distance");
> +    // Old: assert((Range - 2ULL).isNonNegative()&&  "Invalid case distance");
> +    // Why APInt::sge wasn't used?
> +    assert(Range.uge(APInt(Range.getBitWidth(), 2))&&  "Invalid case distance");
> +
>       // Use volatile double here to avoid excess precision issues on some hosts,
>       // e.g. that use 80-bit X87 registers.
>       volatile double LDensity =
> @@ -2407,57 +2410,43 @@
>   /// Clusterify - Transform simple list of Cases into list of CaseRange's
>   size_t SelectionDAGBuilder::Clusterify(CaseVector&  Cases,
>                                          const SwitchInst&  SI) {
> -  size_t numCmps = 0;
> +
> +  /// Use a shorter form of declaration, and also
> +  /// show the we want to use CRSBuilder as Clusterifier.
> +  typedef CRSBuilderBase<MachineBasicBlock, true>  Clusterifier;
> +
> +  Clusterifier TheClusterifier;
>
> -  BranchProbabilityInfo *BPI = FuncInfo.BPI;
>     // Start with "simple" cases
>     for (SwitchInst::ConstCaseIt i = SI.case_begin(), e = SI.case_end();
>          i != e; ++i) {
>       const BasicBlock *SuccBB = i.getCaseSuccessor();
>       MachineBasicBlock *SMBB = FuncInfo.MBBMap[SuccBB];
>
> -    uint32_t ExtraWeight = BPI ? BPI->getEdgeWeight(SI.getParent(), SuccBB) : 0;
> -
> -    Cases.push_back(Case(i.getCaseValue(), i.getCaseValue(),
> -                         SMBB, ExtraWeight));
> +    TheClusterifier.add(i.getCaseValueEx(), SMBB);
>     }
> -  std::sort(Cases.begin(), Cases.end(), CaseCmp());
> -
> -  // Merge case into clusters
> -  if (Cases.size()>= 2)
> -    // Must recompute end() each iteration because it may be
> -    // invalidated by erase if we hold on to it
> -    for (CaseItr I = Cases.begin(), J = llvm::next(Cases.begin());
> -         J != Cases.end(); ) {
> -      const APInt&  nextValue = cast<ConstantInt>(J->Low)->getValue();
> -      const APInt&  currentValue = cast<ConstantInt>(I->High)->getValue();
> -      MachineBasicBlock* nextBB = J->BB;
> -      MachineBasicBlock* currentBB = I->BB;
> -
> -      // If the two neighboring cases go to the same destination, merge them
> -      // into a single case.
> -      if ((nextValue - currentValue == 1)&&  (currentBB == nextBB)) {
> -        I->High = J->High;
> -        J = Cases.erase(J);
> -
> -        if (BranchProbabilityInfo *BPI = FuncInfo.BPI) {
> -          uint32_t CurWeight = currentBB->getBasicBlock() ?
> -            BPI->getEdgeWeight(SI.getParent(), currentBB->getBasicBlock()) : 16;
> -          uint32_t NextWeight = nextBB->getBasicBlock() ?
> -            BPI->getEdgeWeight(SI.getParent(), nextBB->getBasicBlock()) : 16;
> -
> -          BPI->setEdgeWeight(SI.getParent(), currentBB->getBasicBlock(),
> -                             CurWeight + NextWeight);
> -        }
> -      } else {
> -        I = J++;
> -      }
> +
> +  TheClusterifier.optimize();
> +
> +  BranchProbabilityInfo *BPI = FuncInfo.BPI;
> +  size_t numCmps = 0;
> +  for (Clusterifier::RangeIterator i = TheClusterifier.begin(),
> +       e = TheClusterifier.end(); i != e; ++i, ++numCmps) {
> +    Clusterifier::Cluster&C = *i;
> +    unsigned W = 0;
> +    if (BPI) {
> +      W = BPI->getEdgeWeight(SI.getParent(), C.second->getBasicBlock());
> +      if (!W)
> +        W = 16;
> +      W *= C.first.Weight;
> +      BPI->setEdgeWeight(SI.getParent(), C.second->getBasicBlock(), W);
>       }
>
> -  for (CaseItr I=Cases.begin(), E=Cases.end(); I!=E; ++I, ++numCmps) {
> -    if (I->Low != I->High)
> -      // A range counts double, since it requires two compares.
> -      ++numCmps;
> +    Cases.push_back(Case(C.first.Low, C.first.High, C.second, W));
> +
> +    if (C.first.Low != C.first.High)
> +    // A range counts double, since it requires two compares.
> +    ++numCmps;
>     }
>
>     return numCmps;
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits