[DivRemPairs] Add an initial case for hoisting to a common predecessor.

[craig.topper]

Summary

This patch adds support for hoisting the division and maybe the
remainder for control flow graphs like this.

PredBB
  |  \
  |  Rem
  |  /
 Div

If we have DivRem we'll hoist both to PredBB. If not we'll just
hoist Div and expand Rem using the Div.

This improves our codegen for something like this

__uint128_t udivmodti4(__uint128_t dividend, __uint128_t divisor, __uint128_t *remainder) {
    if (remainder != 0)
        *remainder = dividend % divisor;
    return dividend / divisor;
}

[lebedev.ri]

Can we use DominatorTree::findNearestCommonDominator()?
If not, please add a negative test, where it's unsafe to do so.

llvm/lib/Transforms/Scalar/DivRemPairs.cpp

240–241

Remove FIXME?

[craig.topper]

In D87555#2269355, @lebedev.ri wrote:

Can we use DominatorTree::findNearestCommonDominator()?
If not, please add a negative test, where it's unsafe to do so.

The nearest common dominator could put the division on a path that it wasn't going to execute on right? Isn't that not just possibly unsafe but also possibly unprofitable?

llvm/lib/Transforms/Scalar/DivRemPairs.cpp

240–241

I only handled one specific case. There are other cases that could be handled.

[lebedev.ri]

In D87555#2269363, @craig.topper wrote:

In D87555#2269355, @lebedev.ri wrote:

Can we use DominatorTree::findNearestCommonDominator()?
If not, please add a negative test, where it's unsafe to do so.

The nearest common dominator could put the division on a path that it wasn't going to execute on right? Isn't that not just possibly unsafe but also possibly unprofitable?

Yep, that's why i'm asking for a negative test - we then could hoist it past the check for divisor being 0.

[nikic]

Doesn't this need some guaranteed-to-transfer checks for all instructions we're moving across?

[craig.topper]

In D87555#2269381, @nikic wrote:

Doesn't this need some guaranteed-to-transfer checks for all instructions we're moving across?

Good point. Maybe I'll just make sure they are the first instruction for now? That at least covers the case.

[craig.topper]

Ensure that the div/rem are the start of their basic blocks so they are guaranteed to execute.

Still need to add negative tests

[lebedev.ri]

llvm/lib/Transforms/Scalar/DivRemPairs.cpp

260–261

This is too fragile.
You want to check that every instruction between the place you hoist to,
and each div/rem instruction is isGuaranteedToTransferExecutionToSuccessor()

[craig.topper]

Use isGuaranteedToTransferExecutionToSuccessor. Need to be put together negative tests.

[spatel]

I'm not seeing why we need the guaranteed-to-execute checks with the proposed instruction movement. In general, div/rem are special because of their UB problems, but in this case we have:

/// We can largely ignore the normal safety and cost constraints on speculation
/// of these ops when we find a matching pair. This is because we are already
/// guaranteed that any exceptions and most cost are already incurred by the
/// first member of the pair.

In other words, this is similar to hoisting a normal math binop once we find the other member of the pair.

[spatel]

In D87555#2270191, @spatel wrote:

I'm not seeing why we need the guaranteed-to-execute checks with the proposed instruction movement. In general, div/rem are special because of their UB problems, but in this case we have:

/// We can largely ignore the normal safety and cost constraints on speculation
/// of these ops when we find a matching pair. This is because we are already
/// guaranteed that any exceptions and most cost are already incurred by the
/// first member of the pair.

In other words, this is similar to hoisting a normal math binop once we find the other member of the pair.

Ah, never mind. I see it now. We are moving both div and rem.

[RKSimon]

@craig.topper Are you still intending to look at this?

[craig.topper]

-Now with negative tests.
-Support the rem block having PredBB as a predecessor multiple times.

[lebedev.ri]

Seems reasonable to me, but i don't quite recall what was the holdup back then..

llvm/lib/Transforms/Scalar/DivRemPairs.cpp

247

guaranteed

[craig.topper]

Fix typo

[lebedev.ri]

LG, thanks.
@spatel ?

llvm/test/Transforms/DivRemPairs/X86/div-expanded-rem-pair.ll

207

Please precommit the tests

[craig.topper]

Rebase after pre-committing tests

[spatel]

LGTM

[RKSimon]

Thanks Craig!

llvm/lib/Transforms/Scalar/DivRemPairs.cpp

 //===- DivRemPairs.cpp - Hoist/[dr]ecompose division and remainder --------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
for (DivRemPairWorklistEntry &E : Worklist) {
     // already, there's nothing to do. The backend should handle this. If the
     // target does not support div+rem, then we will decompose the rem.
     if (HasDivRemOp && RemInst->getParent() == DivInst->getParent())
       continue;
 
     bool DivDominates = DT.dominates(DivInst, RemInst);
     if (!DivDominates && !DT.dominates(RemInst, DivInst)) {
       // We have matching div-rem pair, but they are in two different blocks,
       // neither of which dominates one another.
-      // FIXME: We could hoist both ops to the common predecessor block?
+
+      BasicBlock *PredBB = nullptr;
+      BasicBlock *DivBB = DivInst->getParent();
+      BasicBlock *RemBB = RemInst->getParent();
+
+      // It's only safe to hoist if every instruction before the Div/Rem in the
+      // basic block is guaranteed to transfer execution.
+      auto IsSafeToHoist = [](Instruction *DivOrRem, BasicBlock *ParentBB) {
+        for (auto I = ParentBB->begin(), E = DivOrRem->getIterator(); I != E;
+             ++I)
+          if (!isGuaranteedToTransferExecutionToSuccessor(&*I))
+            return false;
+
+        return true;
+      };
+
+      // Look for something like this
+      // PredBB
+      //   |  \
+      //   |  Rem
+      //   |  /
+      //  Div
+      //
+      // If the Rem block has a single predecessor and successor, and all paths
+      // from PredBB go to either RemBB or DivBB, and execution of RemBB and
+      // DivBB will always reach the Div/Rem, we can hoist Div to PredBB. If
+      // we have a DivRem operation we can also hoist Rem. Otherwise we'll leave
+      // Rem where it is and rewrite it to mul/sub.
+      // FIXME: We could handle more hoisting cases.
+      if (RemBB->getSingleSuccessor() == DivBB)
+        PredBB = RemBB->getUniquePredecessor();
+
+      if (PredBB && IsSafeToHoist(RemInst, RemBB) &&
+          IsSafeToHoist(DivInst, DivBB) &&
+          llvm::all_of(successors(PredBB), [&](BasicBlock *BB) {
+            return BB == DivBB || BB == RemBB;
+          })) {
+        DivDominates = true;
+        DivInst->moveBefore(PredBB->getTerminator());
+        Changed = true;
+        if (HasDivRemOp) {
+          RemInst->moveBefore(PredBB->getTerminator());
+          continue;
+        }
+      } else
         continue;
     }
 
     // The target does not have a single div/rem operation,
     // and the rem is already in expanded form. Nothing to do.
     if (!HasDivRemOp && E.isRemExpanded())
       continue;
 
     if (HasDivRemOp) {

llvm/test/Transforms/DivRemPairs/X86/div-expanded-rem-pair.ll

 if.else:
   %t3 = udiv i32 %a, %b
   br label %end
 
 end:
   %ret = phi i32 [ %t2, %if.then ], [ %t3, %if.else ]
   ret i32 %ret
 }
+
+; Test for hoisting a udiv to dominate a urem to allow udivrem.
+define i64 @remainder_triangle_i64(i64 %a, i64 %b, i64* %rp) {
+; CHECK-LABEL: @remainder_triangle_i64(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP:%.*]] = icmp ne i64* [[RP:%.*]], null
+; CHECK-NEXT:    [[DIV:%.*]] = udiv i64 [[A:%.*]], [[B:%.*]]
+; CHECK-NEXT:    [[REM:%.*]] = urem i64 [[A]], [[B]]
+; CHECK-NEXT:    br i1 [[CMP]], label [[IF_THEN:%.*]], label [[END:%.*]]
+; CHECK:       if.then:
+; CHECK-NEXT:    store i64 [[REM]], i64* [[RP]], align 4
+; CHECK-NEXT:    br label [[END]]
+; CHECK:       end:
+; CHECK-NEXT:    ret i64 [[DIV]]
+;
+entry:
+  %cmp = icmp ne i64* %rp, null
+  br i1 %cmp, label %if.then, label %end
+
+if.then:
+  %rem = urem i64 %a, %b
+  store i64 %rem, i64* %rp
+  br label %end
+
+end:
+  %div = udiv i64 %a, %b
+  ret i64 %div
+}
+
+; Test for hoisting a udiv to dominate a urem to allow the urem to be expanded
+; into mul+sub.
+define i128 @remainder_triangle_i128(i128 %a, i128 %b, i128* %rp) {
+; CHECK-LABEL: @remainder_triangle_i128(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP:%.*]] = icmp ne i128* [[RP:%.*]], null
+; CHECK-NEXT:    [[A_FROZEN:%.*]] = freeze i128 [[A:%.*]]
+; CHECK-NEXT:    [[B_FROZEN:%.*]] = freeze i128 [[B:%.*]]
+; CHECK-NEXT:    [[DIV:%.*]] = udiv i128 [[A_FROZEN]], [[B_FROZEN]]
+; CHECK-NEXT:    br i1 [[CMP]], label [[IF_THEN:%.*]], label [[END:%.*]]
+; CHECK:       if.then:
+; CHECK-NEXT:    [[TMP0:%.*]] = mul i128 [[DIV]], [[B_FROZEN]]
+; CHECK-NEXT:    [[REM_DECOMPOSED:%.*]] = sub i128 [[A_FROZEN]], [[TMP0]]
+; CHECK-NEXT:    store i128 [[REM_DECOMPOSED]], i128* [[RP]], align 4
+; CHECK-NEXT:    br label [[END]]
+; CHECK:       end:
+; CHECK-NEXT:    ret i128 [[DIV]]
+;
+entry:
+  %cmp = icmp ne i128* %rp, null
+  br i1 %cmp, label %if.then, label %end
+
+if.then:
+  %rem = urem i128 %a, %b
+  store i128 %rem, i128* %rp
+  br label %end
+
+end:
+  %div = udiv i128 %a, %b
+  ret i128 %div
+}
+
+define i64 @remainder_triangle_i64_multiple_rem_edges(i64 %a, i64 %b, i64 %c, i64* %rp) {
+; CHECK-LABEL: @remainder_triangle_i64_multiple_rem_edges(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[DIV:%.*]] = udiv i64 [[A:%.*]], [[B:%.*]]
+; CHECK-NEXT:    [[REM:%.*]] = urem i64 [[A]], [[B]]
+; CHECK-NEXT:    switch i64 [[C:%.*]], label [[SW_DEFAULT:%.*]] [
+; CHECK-NEXT:    i64 0, label [[SW_BB:%.*]]
+; CHECK-NEXT:    i64 2, label [[SW_BB]]
+; CHECK-NEXT:    ]
+; CHECK:       sw.bb:
+; CHECK-NEXT:    store i64 [[REM]], i64* [[RP:%.*]], align 4
+; CHECK-NEXT:    br label [[SW_DEFAULT]]
+; CHECK:       sw.default:
+; CHECK-NEXT:    ret i64 [[DIV]]
+;
+entry:
+  switch i64 %c, label %sw.default [
+  i64 0, label %sw.bb
+  i64 2, label %sw.bb
+  ]
+
+sw.bb:                                            ; preds = %entry, %entry
+  %rem = urem i64 %a, %b
+  store i64 %rem, i64* %rp
+  br label %sw.default
+
+sw.default:                                       ; preds = %entry, %sw.bb
+  %div = udiv i64 %a, %b
+  ret i64 %div
+}
+
+define i64 @remainder_triangle_i64_multiple_div_edges(i64 %a, i64 %b, i64 %c, i64* %rp) {
+; CHECK-LABEL: @remainder_triangle_i64_multiple_div_edges(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[DIV:%.*]] = udiv i64 [[A:%.*]], [[B:%.*]]
+; CHECK-NEXT:    [[REM:%.*]] = urem i64 [[A]], [[B]]
+; CHECK-NEXT:    switch i64 [[C:%.*]], label [[SW_DEFAULT:%.*]] [
+; CHECK-NEXT:    i64 0, label [[SW_BB:%.*]]
+; CHECK-NEXT:    i64 2, label [[SW_BB]]
+; CHECK-NEXT:    ]
+; CHECK:       sw.default:
+; CHECK-NEXT:    store i64 [[REM]], i64* [[RP:%.*]], align 4
+; CHECK-NEXT:    br label [[SW_BB]]
+; CHECK:       sw.bb:
+; CHECK-NEXT:    ret i64 [[DIV]]
+;
+entry:
+  switch i64 %c, label %sw.default [
+  i64 0, label %sw.bb
+  i64 2, label %sw.bb
+  ]
+
+sw.default:                                       ; preds = %entry, %entry
+  %rem = urem i64 %a, %b
+  store i64 %rem, i64* %rp
+  br label %sw.bb
+
+sw.bb:                                            ; preds = %entry, %sw.default
+  %div = udiv i64 %a, %b
+  ret i64 %div
+}
+
+declare void @maythrow()
+
+; Negative test. make sure we don't transform if there are instructions before
+; the rem that might throw.
+define i64 @remainder_triangle_i64_maythrow_rem(i64 %a, i64 %b, i64* %rp) {
+; CHECK-LABEL: @remainder_triangle_i64_maythrow_rem(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP:%.*]] = icmp ne i64* [[RP:%.*]], null
+; CHECK-NEXT:    br i1 [[CMP]], label [[IF_THEN:%.*]], label [[END:%.*]]
+; CHECK:       if.then:
+; CHECK-NEXT:    call void @maythrow()
+; CHECK-NEXT:    [[REM:%.*]] = urem i64 [[A:%.*]], [[B:%.*]]
+; CHECK-NEXT:    store i64 [[REM]], i64* [[RP]], align 4
+; CHECK-NEXT:    br label [[END]]
+; CHECK:       end:
+; CHECK-NEXT:    [[DIV:%.*]] = udiv i64 [[A]], [[B]]
+; CHECK-NEXT:    ret i64 [[DIV]]
+;
+entry:
+  %cmp = icmp ne i64* %rp, null
+  br i1 %cmp, label %if.then, label %end
+
+if.then:
+  call void @maythrow()
+  %rem = urem i64 %a, %b
+  store i64 %rem, i64* %rp
+  br label %end
+
+end:
+  %div = udiv i64 %a, %b
+  ret i64 %div
+}
+
+; Negative test. make sure we don't transform if there are instructions before
+; the div that might throw.
+define i64 @remainder_triangle_i64_maythrow_div(i64 %a, i64 %b, i64* %rp) {
+; CHECK-LABEL: @remainder_triangle_i64_maythrow_div(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP:%.*]] = icmp ne i64* [[RP:%.*]], null
+; CHECK-NEXT:    br i1 [[CMP]], label [[IF_THEN:%.*]], label [[END:%.*]]
+; CHECK:       if.then:
+; CHECK-NEXT:    [[REM:%.*]] = urem i64 [[A:%.*]], [[B:%.*]]
+; CHECK-NEXT:    store i64 [[REM]], i64* [[RP]], align 4
+; CHECK-NEXT:    br label [[END]]
+; CHECK:       end:
+; CHECK-NEXT:    call void @maythrow()
+; CHECK-NEXT:    [[DIV:%.*]] = udiv i64 [[A]], [[B]]
+; CHECK-NEXT:    ret i64 [[DIV]]
+;
+entry:
+  %cmp = icmp ne i64* %rp, null
+  br i1 %cmp, label %if.then, label %end
+
+if.then:
+  %rem = urem i64 %a, %b
+  store i64 %rem, i64* %rp
+  br label %end
+
+end:
+  call void @maythrow()
+  %div = udiv i64 %a, %b
+  ret i64 %div
+}
+
+; Negative test, Make sure we don't transform if there are instructions before
+; the rem that might throw.
+define i128 @remainder_triangle_i128_maythrow_rem(i128 %a, i128 %b, i128* %rp) {
+; CHECK-LABEL: @remainder_triangle_i128_maythrow_rem(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP:%.*]] = icmp ne i128* [[RP:%.*]], null
+; CHECK-NEXT:    br i1 [[CMP]], label [[IF_THEN:%.*]], label [[END:%.*]]
+; CHECK:       if.then:
+; CHECK-NEXT:    call void @maythrow()
+; CHECK-NEXT:    [[REM:%.*]] = urem i128 [[A:%.*]], [[B:%.*]]
+; CHECK-NEXT:    store i128 [[REM]], i128* [[RP]], align 4
+; CHECK-NEXT:    br label [[END]]
+; CHECK:       end:
+; CHECK-NEXT:    [[DIV:%.*]] = udiv i128 [[A]], [[B]]
+; CHECK-NEXT:    ret i128 [[DIV]]
+;
+entry:
+  %cmp = icmp ne i128* %rp, null
+  br i1 %cmp, label %if.then, label %end
+
+if.then:
+  call void @maythrow()
+  %rem = urem i128 %a, %b
+  store i128 %rem, i128* %rp
+  br label %end
+
+end:
+  %div = udiv i128 %a, %b
+  ret i128 %div
+}
+
+; Negative test. Make sure we don't transform if there are instructions before
+; the div that might throw.
+define i128 @remainder_triangle_i128_maythrow_div(i128 %a, i128 %b, i128* %rp) {
+; CHECK-LABEL: @remainder_triangle_i128_maythrow_div(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP:%.*]] = icmp ne i128* [[RP:%.*]], null
+; CHECK-NEXT:    br i1 [[CMP]], label [[IF_THEN:%.*]], label [[END:%.*]]
+; CHECK:       if.then:
+; CHECK-NEXT:    [[REM:%.*]] = urem i128 [[A:%.*]], [[B:%.*]]
+; CHECK-NEXT:    store i128 [[REM]], i128* [[RP]], align 4
+; CHECK-NEXT:    br label [[END]]
+; CHECK:       end:
+; CHECK-NEXT:    call void @maythrow()
+; CHECK-NEXT:    [[DIV:%.*]] = udiv i128 [[A]], [[B]]
+; CHECK-NEXT:    ret i128 [[DIV]]
+;
+entry:
+  %cmp = icmp ne i128* %rp, null
+  br i1 %cmp, label %if.then, label %end
+
+if.then:
+  %rem = urem i128 %a, %b
+  store i128 %rem, i128* %rp
+  br label %end
+
+end:
+  call void @maythrow()
+  %div = udiv i128 %a, %b
+  ret i128 %div
+}
+
+; Negative test. The common predecessor has another successor so we can't hoist
+; the udiv to the common predecessor.
+define i64 @remainder_not_triangle_i32(i64 %a, i64 %b, i64 %c, i64*%rp) {
+; CHECK-LABEL: @remainder_not_triangle_i32(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    switch i64 [[C:%.*]], label [[RETURN:%.*]] [
+; CHECK-NEXT:    i64 0, label [[SW_BB:%.*]]
+; CHECK-NEXT:    i64 1, label [[SW_BB1:%.*]]
+; CHECK-NEXT:    ]
+; CHECK:       sw.bb:
+; CHECK-NEXT:    [[REM:%.*]] = urem i64 [[A:%.*]], [[B:%.*]]
+; CHECK-NEXT:    store i64 [[REM]], i64* [[RP:%.*]], align 4
+; CHECK-NEXT:    br label [[SW_BB1]]
+; CHECK:       sw.bb1:
+; CHECK-NEXT:    [[DIV:%.*]] = udiv i64 [[A]], [[B]]
+; CHECK-NEXT:    br label [[RETURN]]
+; CHECK:       return:
+; CHECK-NEXT:    [[RETVAL_0:%.*]] = phi i64 [ [[DIV]], [[SW_BB1]] ], [ 0, [[ENTRY:%.*]] ]
+; CHECK-NEXT:    ret i64 [[RETVAL_0]]
+;
+entry:
+  switch i64 %c, label %return [
+  i64 0, label %sw.bb
+  i64 1, label %sw.bb1
+  ]
+
+sw.bb:                                            ; preds = %entry
+  %rem = urem i64 %a, %b
+  store i64 %rem, i64* %rp
+  br label %sw.bb1
+
+sw.bb1:                                           ; preds = %entry, %sw.bb
+  %div = udiv i64 %a, %b
+  br label %return
+
+return:                                           ; preds = %entry, %sw.bb1
+  %retval.0 = phi i64 [ %div, %sw.bb1 ], [ 0, %entry ]
+  ret i64 %retval.0
+}
+
+; Negative test. The urem block has a successor that isn't udiv.
+define i64 @remainder_not_triangle_i32_2(i64 %a, i64 %b, i64 %c, i64* %rp) {
+; CHECK-LABEL: @remainder_not_triangle_i32_2(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TOBOOL_NOT:%.*]] = icmp eq i64* [[RP:%.*]], null
+; CHECK-NEXT:    br i1 [[TOBOOL_NOT]], label [[IF_END3:%.*]], label [[IF_THEN:%.*]]
+; CHECK:       if.then:
+; CHECK-NEXT:    [[REM:%.*]] = urem i64 [[A:%.*]], [[B:%.*]]
+; CHECK-NEXT:    store i64 [[REM]], i64* [[RP]], align 4
+; CHECK-NEXT:    [[TOBOOL1_NOT:%.*]] = icmp eq i64 [[C:%.*]], 0
+; CHECK-NEXT:    br i1 [[TOBOOL1_NOT]], label [[IF_END3]], label [[RETURN:%.*]]
+; CHECK:       if.end3:
+; CHECK-NEXT:    [[DIV:%.*]] = udiv i64 [[A]], [[B]]
+; CHECK-NEXT:    br label [[RETURN]]
+; CHECK:       return:
+; CHECK-NEXT:    [[RETVAL_0:%.*]] = phi i64 [ [[DIV]], [[IF_END3]] ], [ 0, [[IF_THEN]] ]
+; CHECK-NEXT:    ret i64 [[RETVAL_0]]
+;
+entry:
+  %tobool.not = icmp eq i64* %rp, null
+  br i1 %tobool.not, label %if.end3, label %if.then
+
+if.then:                                          ; preds = %entry
+  %rem = urem i64 %a, %b
+  store i64 %rem, i64* %rp
+  %tobool1.not = icmp eq i64 %c, 0
+  br i1 %tobool1.not, label %if.end3, label %return
+
+if.end3:                                          ; preds = %if.then, %entry
+  %div = udiv i64 %a, %b
+  br label %return
+
+return:                                           ; preds = %if.then, %if.end3
+  %retval.0 = phi i64 [ %div, %if.end3 ], [ 0, %if.then ]
+  ret i64 %retval.0
+}