[llvm-commits] [polly] r143576 - /polly/trunk/lib/Analysis/ScopDetection.cpp

[Tobias Grosser]

On 11/16/2011 05:38 AM, Sebastian Pop wrote:
> On Wed, Nov 2, 2011 at 4:40 PM, Tobias Grosser
> <grosser at fim.uni-passau.de>  wrote:
>> Author: grosser
>> Date: Wed Nov  2 16:40:08 2011
>> New Revision: 143576
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=143576&view=rev
>> Log:
>> ScopDetection: Add new SCEV Validator
>>
>> The SCEV Validator is used to check if the bound of a loop can be translated
>> into a polyhedral constraint. The new validator is more general as the check
>> used previously and e.g. allows bounds like 'smax 1, %a'. At the moment, we
>> only allow signed comparisons. Also, the new validator is only used to verify
>> loop bounds. Memory accesses are still handled by the old validator.
>>
>> Modified:
>>     polly/trunk/lib/Analysis/ScopDetection.cpp
>>
>> Modified: polly/trunk/lib/Analysis/ScopDetection.cpp
>> URL: http://llvm.org/viewvc/llvm-project/polly/trunk/lib/Analysis/ScopDetection.cpp?rev=143576&r1=143575&r2=143576&view=diff
>> ==============================================================================
>> --- polly/trunk/lib/Analysis/ScopDetection.cpp (original)
>> +++ polly/trunk/lib/Analysis/ScopDetection.cpp Wed Nov  2 16:40:08 2011
>> @@ -109,6 +109,162 @@
>>   //===----------------------------------------------------------------------===//
>>   // ScopDetection.
>>
>> +namespace SCEVType {
>> +  enum TYPE {INT, PARAM, IV, INVALID};
>> +}
>> +
>> +/// Check if a SCEV is valid in a SCoP.
>> +struct SCEVValidator : public SCEVVisitor<SCEVValidator, SCEVType::TYPE>  {
>> +private:
>> +  const Region *R;
>> +  ScalarEvolution&SE;
>> +  const Value **BaseAddress;
>> +
>> +public:
>> +  static bool isValid(const Region *R, const SCEV *Scev,
>> +                      ScalarEvolution&SE,
>> +                      const Value **BaseAddress = NULL) {
>> +    if (isa<SCEVCouldNotCompute>(Scev))
>> +      return false;
>> +
>> +    SCEVValidator Validator(R, SE, BaseAddress);
>> +    return Validator.visit(Scev) != SCEVType::INVALID;
>> +  }
>> +
>> +  SCEVValidator(const Region *R, ScalarEvolution&SE,
>> +                const Value **BaseAddress) : R(R), SE(SE),
>> +    BaseAddress(BaseAddress) {};
>> +
>> +  SCEVType::TYPE visitConstant(const SCEVConstant *Constant) {
>> +    return SCEVType::INT;
>> +  }
>> +
>> +  SCEVType::TYPE visitTruncateExpr(const SCEVTruncateExpr* Expr) {
>
> Please use a single way to place the *, i.e., SCEVTruncateExpr *Expr
> also occurs below, several times in several inconsistent ways.

Fixed in "Fix placement of the '*' that marks a pointer"


>> +    SCEVType::TYPE Op = visit(Expr->getOperand());
>> +
>> +    // We cannot represent this as a affine expression yet. If it is constant
>
> s/a affine/an affine/
>
>> +    // during Scop execution treat this as a parameter, otherwise bail out.
>> +    if (Op == SCEVType::INT || Op == SCEVType::PARAM)
>> +      return SCEVType::PARAM;
>> +
>> +    return SCEVType::INVALID;
>> +  }
>> +
>> +  SCEVType::TYPE visitZeroExtendExpr(const SCEVZeroExtendExpr * Expr) {
>> +    SCEVType::TYPE Op = visit(Expr->getOperand());
>> +
>> +    // We cannot represent this as a affine expression yet. If it is constant
>
> s/a affine/an affine/

This was already fixed in a later commit.

>> +    // during Scop execution treat this as a parameter, otherwise bail out.
>> +    if (Op == SCEVType::INT || Op == SCEVType::PARAM)
>> +      return SCEVType::PARAM;
>> +
>> +    return SCEVType::INVALID;
>> +  }
>> +
>> +  SCEVType::TYPE visitSignExtendExpr(const SCEVSignExtendExpr* Expr) {
>> +    // Assuming the value is signed, a sign extension is basically a noop.
>> +    // TODO: Reconsider this as soon as we support unsigned values.
>> +    return visit(Expr->getOperand());
>> +  }
>> +
>> +  SCEVType::TYPE visitAddExpr(const SCEVAddExpr* Expr) {
>> +    SCEVType::TYPE Return = SCEVType::INT;
>> +
>> +    for (int i = 0, e = Expr->getNumOperands(); i<  e; ++i) {
>> +      SCEVType::TYPE OpType = visit(Expr->getOperand(i));
>> +      Return = std::max(Return, OpType);
>> +    }
>> +
>> +    // TODO: Check for NSW and NUW.
>> +    return Return;
>> +  }
>> +
>> +  SCEVType::TYPE visitMulExpr(const SCEVMulExpr* Expr) {
>> +    SCEVType::TYPE Return = SCEVType::INT;
>> +
>> +    for (int i = 0, e = Expr->getNumOperands(); i<  e; ++i) {
>> +      SCEVType::TYPE OpType = visit(Expr->getOperand(i));
>> +
>> +      if (OpType == SCEVType::INVALID)
>> +        return SCEVType::INVALID;
>> +
>> +      if (OpType == SCEVType::IV) {
>> +        if (Return == SCEVType::PARAM || Return == SCEVType::IV)
>> +          return SCEVType::INVALID;
>> +
>> +        Return = OpType;
>> +        continue;
>> +      }
>> +
>> +      if (OpType == SCEVType::PARAM) {
>> +        if (Return == SCEVType::PARAM)
>> +          return SCEVType::INVALID;
>> +
>> +        Return = SCEVType::PARAM;
>> +        continue;
>> +      }
>> +
>> +      // OpType == SCEVType::INT, no need to change anything.
>> +    }
>> +
>> +    // TODO: Check for NSW and NUW.
>> +    return Return;
>> +  }
>> +
>> +  SCEVType::TYPE visitUDivExpr(const SCEVUDivExpr* Expr) {
>> +    // We do not yet support unsigned operations.
>> +    return SCEVType::INVALID;
>> +  }
>> +
>> +  SCEVType::TYPE visitAddRecExpr(const SCEVAddRecExpr* Expr) {
>> +    if (!Expr->isAffine())
>> +      return SCEVType::INVALID;
>> +
>> +    SCEVType::TYPE Start = visit(Expr->getStart());
>> +
>> +    if (Start == SCEVType::INVALID)
>> +      return Start;
>> +
>> +    SCEVType::TYPE Recurrence = visit(Expr->getStepRecurrence(SE));
>
> In the following code, why not doing this instead?
>
>
>     if (Recurrence == SCEVType::INT)
>       return SCEVType::IV;
>
>     return SCEVType::INVALID;

I used the chance to reorganize the structure of the whole function.
I hope it is now better to understand:

Committed in "SCEVValidator: Restructure the logic of visitAddRecExpr"

>> +    if (Recurrence != SCEVType::INT)
>> +      return SCEVType::INVALID;
>> +
>> +    return SCEVType::PARAM;
>
> I don't see why you would return a PARAM for an addrec with a constant step.
> I would return SCEVType::IV.

This one is already fixed in trunk.
>
>> +  }
>> +
>> +  SCEVType::TYPE visitSMaxExpr(const SCEVSMaxExpr* Expr) {
>> +    SCEVType::TYPE Return = SCEVType::INT;
>> +
>> +    for (int i = 0, e = Expr->getNumOperands(); i<  e; ++i) {
>> +      SCEVType::TYPE OpType = visit(Expr->getOperand(i));
>> +
>> +      if (OpType == SCEVType::INVALID)
>> +        return SCEVType::INVALID;
>> +      if (OpType == SCEVType::PARAM)
>> +        Return = SCEVType::PARAM;
>> +    }
>> +
>> +    return Return;
>> +  }
>> +
>> +  SCEVType::TYPE visitUMaxExpr(const SCEVUMaxExpr* Expr) {
>> +    // We do not yet support unsigned operations. If 'Expr' is constant
>> +    // during Scop execution treat this as a parameter, otherwise bail out.
>> +    for (int i = 0, e = Expr->getNumOperands(); i<  e; ++i) {
>> +      SCEVType::TYPE OpType = visit(Expr->getOperand(i));
>> +
>> +      if (OpType != SCEVType::INT&&  OpType != SCEVType::PARAM)
>> +        return SCEVType::PARAM;
>
> Probably this first return should have been SCEVType::INVALID?
Yes, already fixed in trunk.

>
>> +    }
>> +
>> +    return SCEVType::PARAM;
>> +  }
>
> You should check the logic in this function, as I see that you always
> return SCEVType::PARAM.

Already fixed in trunk.

>> +
>> +  SCEVType::TYPE visitUnknown(const SCEVUnknown* Expr) {
>> +    return SCEVType::PARAM;
>> +  }
>
> Really?  I would have returned INVALID instead of PARAM for an unknown SCEV.

;-)

Unknown is the name SCEV uses for valid variables where the actual value
is unknown. This means SCEVUnknowns are references to llvm::Value* and 
represent e.g. variables like '%a'. So yes, an SCEVUnknown is definitely 
a parameter.

Thanks for the review
Tobi