[llvm-commits] [llvm] r156804 - /llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
Stepan Dyatkovskiy
stpworld at narod.ru
Tue May 15 02:42:29 PDT 2012
Hi Duncan.
Well. Nobody was reviewed this changes, since I commited it for
post-commit review. I was sure that it will work fine, since I learned
how SelectionDAGBuilder::handleJTSwitchCase works and was sure in my
patch. We just here we need to produce jump table like this:
Offset = Case0.Low
[0] = Case0.Successor
...
[Case0.High - Offset] = Case0.Successor
[Case0.High - Offset + 1] = Default // Doesn't belongs to any range.
[Case0.High - Offset + 2] = Default
...
[Case1.Low - 1 - Offset] = Default
[Case1.Low - Offset] = Case1.Successor
...
[Case1.High - Offset] = Case1.Successor
[Case1.High - Offset + 1] = Default
...
Currently I'm trying to reproduce at least something on my local machine
and debug it. I also will glad if somebody (Anton?) will reviewed my code.
Relative to
> if (Low.ule(TEI)&& TEI.ule(High))
Probably it is a not best way to check whether TEI belongs to range, but
I don't think that itsa point of bug.
Probably replacement of comparison from signed to unsigned brakes it
with someway.
-Stepan.
Duncan Sands wrote:
> 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
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list