[polly] r271898 - Refactor division generation code

[Tobias Grosser via llvm-commits]

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.

This patch seems to mix a refactoring with two functional changes. In
general I would prefer for functional changes to not be mixed and if
possible even for refactorings to be moved out first.

> 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.

> +  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?

Best,
Tobias