[llvm] r307554 - Fix invalid cast in instcombine UMul/ZExt idiom

[Davide Italiano via llvm-commits]

On Mon, Jul 10, 2017 at 9:51 AM, Serge Guelton via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
> Author: serge_sans_paille
> Date: Mon Jul 10 09:51:40 2017
> New Revision: 307554
>
> URL: http://llvm.org/viewvc/llvm-project?rev=307554&view=rev
> Log:
> Fix invalid cast in instcombine UMul/ZExt idiom
>
> Fixes https://bugs.llvm.org/show_bug.cgi?id=25454
>
> Do not assume IRBuilder creates Instruction where it can create Value.
> Do not assume idiom operands are constant, leave generalisation ot the IRBuilder.
>
> Differential Revision: https://reviews.llvm.org/D35114
>
> Added:
>     llvm/trunk/test/Transforms/InstCombine/2017-07-07-UMul-ZExt.ll
> Modified:
>     llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
>
> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp?rev=307554&r1=307553&r2=307554&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp (original)
> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp Mon Jul 10 09:51:40 2017
> @@ -3856,17 +3856,18 @@ static Instruction *processUMulZExtIdiom
>        } else if (BinaryOperator *BO = dyn_cast<BinaryOperator>(U)) {
>          assert(BO->getOpcode() == Instruction::And);
>          // Replace (mul & mask) --> zext (mul.with.overflow & short_mask)
> -        ConstantInt *CI = cast<ConstantInt>(BO->getOperand(1));
> -        APInt ShortMask = CI->getValue().trunc(MulWidth);
> +        Value *ShortMask =
> +            Builder.CreateTrunc(BO->getOperand(1), Builder.getIntNTy(MulWidth));

I'm afraid this bit is not quite right.
`Bo->getOperand(1)` could live in another block, consider e.g.

@glob = external global i16

define void @patatino(i8 %beth) {
  %conv = zext i8 %beth to i32
  %mul = mul nuw nsw i32 %conv, %conv
  %conv3 = and i32 %mul, 255
  %tobool8 = icmp ne i32 %mul, %conv3
  br i1 %tobool8, label %if.then9, label %if.then9

if.then9:                                         ; preds = %0, %0
  %tinky = load i16, i16* @glob
  %conv13 = sext i16 %tinky to i32
  %and = and i32 %mul, %conv13
  %conv14 = trunc i32 %and to i16
  store i16 %conv14, i16* @glob
  ret void
}

after this transformation would become:


define void @patatino(i8 %beth) {
  %conv = zext i8 %beth to i32
  %umul = call { i8, i1 } @llvm.umul.with.overflow.i8(i8 %beth, i8 %beth)
  %umul.value = extractvalue { i8, i1 } %umul, 0
  %1 = trunc i32 %conv13 to i8
  %2 = and i8 %umul.value, %1
  %3 = zext i8 %2 to i32
  %mul = mul nuw nsw i32 %conv, %conv
  %conv3 = and i32 %mul, 255
  %tobool8 = extractvalue { i8, i1 } %umul, 1
  br i1 %tobool8, label %if.then9, label %if.then9

if.then9:                                         ; preds = %0, %0
  %tinky = load i16, i16* @glob
  %conv13 = sext i16 %tinky to i32
  %and = and i32 %mul, %conv13
  %conv14 = trunc i32 %3 to i16
  store i16 %conv14, i16* @glob
  ret void
}

Note how you're inserting `%1` which has an operand an use of
`%conv13` which is defined in `%if.then9`, i.e. you end up with a def
that doesn't dominate all the uses -> violates SSA.

I'm going to post a patch to fix this for review soon.

--
Davide