[polly] r271898 - Refactor division generation code

[Tobias Grosser via llvm-commits]

On 06/07/2016 07:07 PM, Tobias Grosser wrote:
> On 06/06/2016 04:56 PM, Johannes Doerfert via llvm-commits wrote:
>> Author: jdoerfert
>> Date: Mon Jun  6 09:56:17 2016
>> New Revision: 271898
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=271898&view=rev
>> Log:
>> Refactor division generation code
>>
>>   This patch refactors the code generation for divisions. This allows to
>>   always generate a shift for a power-of-two division and to utilize
>>   information about constant divisors in order to truncate the result
>>   type.

Hi Johannes,

I unfortunately had to revert this commit as part of the revert in
272483, as I had some conflicts. As this change is mostly unrelated I
would like to recommit it as soon as possible to make sure it is not
outdated.

As I also had some comments on this patch, it would probably be best to
address them before we recommit this patch.

>> Modified:
>>     polly/trunk/include/polly/CodeGen/IslExprBuilder.h
>>     polly/trunk/lib/CodeGen/IslExprBuilder.cpp
>>     polly/trunk/test/Isl/CodeGen/exprModDiv.ll
>>     polly/trunk/test/Isl/CodeGen/invariant_load_escaping_second_scop.ll
>>
>> Modified: polly/trunk/include/polly/CodeGen/IslExprBuilder.h
>> URL: http://llvm.org/viewvc/llvm-project/polly/trunk/include/polly/CodeGen/IslExprBuilder.h?rev=271898&r1=271897&r2=271898&view=diff
>> ==============================================================================
>> --- polly/trunk/include/polly/CodeGen/IslExprBuilder.h (original)
>> +++ polly/trunk/include/polly/CodeGen/IslExprBuilder.h Mon Jun  6 09:56:17 2016
>> @@ -204,6 +204,9 @@ private:
>>    llvm::Value *createOpAddressOf(__isl_take isl_ast_expr *Expr);
>>    llvm::Value *createAccessAddress(__isl_take isl_ast_expr *Expr);
>>  
>> +  /// @brief Supported kinds of division.
>> +  enum DivisionMode { DM_SIGNED, DM_UNSIGNED, DM_FLOORED };
> 
>                                                  DM_FLOORD?
> 
> Why do you introduce a new enum here? Does isl_ast_expr_type not work? I
> personally believe this new enum adds confusion by adding an additional
> indirection.
> 
>> +Value *IslExprBuilder::createDiv(Value *LHS, Value *RHS, DivisionMode DM) {
>> +  auto *ConstRHS = dyn_cast<ConstantInt>(RHS);
>> +  unsigned UnusedBits = 0;
>> +  Value *Res = nullptr;
>> +
>> +  if (ConstRHS)
>> +    UnusedBits = ConstRHS->getValue().logBase2();
>> +
>> +  if (ConstRHS && ConstRHS->getValue().isPowerOf2() &&
>> +      ConstRHS->getValue().isNonNegative())
>> +    Res = Builder.CreateAShr(LHS, UnusedBits, "polly.div.shr");
> 
> As the old code could easily have lowered isl_ast_op_div to ASHR before,
> it would be good to explain in the commit message why using ASHR for
> isl_ast_op_div is correct and why the old code probably just missed this
> opportunity. The current commit message does not make clear why you
> believe this transformation is correct, hence this could just
> be an accidental change. As a result, a post-commit reviewer has to dig
> into this issue himself, which makes post-commit reviews more
> time-consuming.

Could you give a brief explanation why this change is correct, such that
this can be added to the commit message?

>> +  else if (DM == DM_SIGNED)
>> +    Res = Builder.CreateSDiv(LHS, RHS, "pexp.div", true);
>> +  else if (DM == DM_UNSIGNED)
>> +    Res = Builder.CreateUDiv(LHS, RHS, "pexp.p_div_q");
>> +  else {
>> +    assert(DM == DM_FLOORED);
>> +    // TODO: Review code and check that this calculation does not yield
>> +    //       incorrect overflow in some bordercases.
>> +    //
>> +    // floord(n,d) ((n < 0) ? (n - d + 1) : n) / d
>> +    Value *Sum1 = createSub(LHS, RHS, "pexp.fdiv_q.0");
>> +    Value *One = ConstantInt::get(Sum1->getType(), 1);
>> +    Value *Sum2 = createAdd(Sum1, One, "pexp.fdiv_q.1");
>> +    Value *Zero = ConstantInt::get(LHS->getType(), 0);
>> +    Value *isNegative = Builder.CreateICmpSLT(LHS, Zero, "pexp.fdiv_q.2");
>> +    unifyTypes(LHS, Sum2);
>> +    Value *Dividend =
>> +        Builder.CreateSelect(isNegative, Sum2, LHS, "pexp.fdiv_q.3");
>> +    unifyTypes(Dividend, RHS);
>> +    Res = Builder.CreateSDiv(Dividend, RHS, "pexp.fdiv_q.4");
>> +  }
>> +
>> +  if (UnusedBits) {
>> +    auto RequiredBits = Res->getType()->getPrimitiveSizeInBits() - UnusedBits;
>> +    Res = Builder.CreateTrunc(Res, Builder.getIntNTy(RequiredBits),
>> +                              "polly.div.trunc");
>> +  }
> 
> The last change seems unrelated and should be part of a separate commit.
> I also do not understand the motivation of why we would want to use less
> bits here? Truncating to a non-native type and expanding later will
> introduce additional complexity when lowering to native code. Could you
> explain your reasoning?

I would suggest to drop this for now to make this patch independent of
the type-size changes.

Best,
Tobias