[PATCH] D140647: Handle simple diamond CFG hoisting in DivRemPairs.

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Can you please add these additional tests for the invoke/catchswitch cases?

  declare void @dummy()
  
  define i32 @invoke_not_willreturn(i32 %a, i32 %b) personality ptr null {
  ; CHECK-LABEL: @invoke_not_willreturn(
  ; CHECK-NEXT:  entry:
  ; CHECK-NEXT:    invoke void @dummy()
  ; CHECK-NEXT:    to label [[CONT:%.*]] unwind label [[LPAD:%.*]]
  ; CHECK:       cont:
  ; CHECK-NEXT:    [[DIV:%.*]] = sdiv i32 [[A:%.*]], [[B:%.*]]
  ; CHECK-NEXT:    ret i32 [[DIV]]
  ; CHECK:       lpad:
  ; CHECK-NEXT:    [[TMP0:%.*]] = landingpad { ptr, i32 }
  ; CHECK-NEXT:    cleanup
  ; CHECK-NEXT:    [[REM:%.*]] = srem i32 [[A]], [[B]]
  ; CHECK-NEXT:    ret i32 [[REM]]
  ;
  entry:
    invoke void @dummy()
    to label %cont unwind label %lpad
  
  cont:
    %div = sdiv i32 %a, %b
    ret i32 %div
  
  lpad:
    landingpad { ptr, i32 }
    cleanup
    %rem = srem i32 %a, %b
    ret i32 %rem
  }
  
  define i32 @invoke_willreturn(i32 %a, i32 %b) personality ptr null {
  ; CHECK-LABEL: @invoke_willreturn(
  ; CHECK-NEXT:  entry:
  ; CHECK-NEXT:    [[DIV:%.*]] = sdiv i32 [[A:%.*]], [[B:%.*]]
  ; CHECK-NEXT:    [[REM:%.*]] = srem i32 [[A]], [[B]]
  ; CHECK-NEXT:    invoke void @dummy() #[[ATTR0:[0-9]+]]
  ; CHECK-NEXT:    to label [[CONT:%.*]] unwind label [[LPAD:%.*]]
  ; CHECK:       cont:
  ; CHECK-NEXT:    ret i32 [[DIV]]
  ; CHECK:       lpad:
  ; CHECK-NEXT:    [[TMP0:%.*]] = landingpad { ptr, i32 }
  ; CHECK-NEXT:    cleanup
  ; CHECK-NEXT:    ret i32 [[REM]]
  ;
  entry:
    invoke void @dummy() willreturn
    to label %cont unwind label %lpad
  
  cont:
    %div = sdiv i32 %a, %b
    ret i32 %div
  
  lpad:
    landingpad { ptr, i32 }
    cleanup
    %rem = srem i32 %a, %b
    ret i32 %rem
  }
  
  ; Use this personality function so that catchpad is guaranteed to transfer.
  declare void @ProcessCLRException()
  
  define i32 @catchswitch(i32 %a, i32 %b) personality ptr @ProcessCLRException {
  ; CHECK-LABEL: @catchswitch(
  ; CHECK-NEXT:  entry:
  ; CHECK-NEXT:    invoke void @dummy() #[[ATTR0]]
  ; CHECK-NEXT:    to label [[CONT:%.*]] unwind label [[LPAD:%.*]]
  ; CHECK:       cont:
  ; CHECK-NEXT:    ret i32 0
  ; CHECK:       lpad:
  ; CHECK-NEXT:    [[CS:%.*]] = catchswitch within none [label %cp] unwind label [[LPAD_END:%.*]]
  ; CHECK:       cp:
  ; CHECK-NEXT:    [[TMP0:%.*]] = catchpad within [[CS]] []
  ; CHECK-NEXT:    [[DIV:%.*]] = sdiv i32 [[A:%.*]], [[B:%.*]]
  ; CHECK-NEXT:    ret i32 [[DIV]]
  ; CHECK:       lpad.end:
  ; CHECK-NEXT:    [[TMP1:%.*]] = cleanuppad within none []
  ; CHECK-NEXT:    [[REM:%.*]] = srem i32 [[A]], [[B]]
  ; CHECK-NEXT:    ret i32 [[REM]]
  ;
  entry:
    invoke void @dummy() willreturn
    to label %cont unwind label %lpad
  
  cont:
    ret i32 0
  
  lpad:
    %cs = catchswitch within none [label %cp] unwind label %lpad.end
  
  cp:
    catchpad within %cs []
    %div = sdiv i32 %a, %b
    ret i32 %div
  
  lpad.end:
    cleanuppad within none []
    %rem = srem i32 %a, %b
    ret i32 %rem
  }

The catchswitch exception was a bit tricky to come up with, because we need both the right personality function and no unwind to caller, otherwise we will fail transfer checks already.



================
Comment at: llvm/lib/Transforms/Scalar/DivRemPairs.cpp:284
+      // sooner. Without a DivRem op, this transformation is unprofitable because
+      // we would end up performing an extra Div on the Rem path.
+      } else if (BasicBlock* RemPredBB = RemBB->getUniquePredecessor()) {
----------------
"extra Div" -> Mul + Sub? I don't think we'd perform an actual div.


================
Comment at: llvm/lib/Transforms/Scalar/DivRemPairs.cpp:285
+      // we would end up performing an extra Div on the Rem path.
+      } else if (BasicBlock* RemPredBB = RemBB->getUniquePredecessor()) {
+        // This hoist is only profitable when the target has a DivRem op.
----------------
nit: `BasicBlock *`


================
Comment at: llvm/lib/Transforms/Scalar/DivRemPairs.cpp:287
+        // This hoist is only profitable when the target has a DivRem op.
+        if (HasDivRemOp && RemPredBB == DivBB->getUniquePredecessor()) {
+          PredBB = RemPredBB;
----------------
nit: No braces for single-line if.


================
Comment at: llvm/lib/Transforms/Scalar/DivRemPairs.cpp:294
+      if (PredBB &&
+          !PredBB->isEHPad() &&
+          isGuaranteedToTransferExecutionToSuccessor(PredBB->getTerminator()) &&
----------------
Hm, this is stricter than what is needed, most exception pads are fine. It looks like we don't really have a dedicated method for this, so just checking `!isa<CatchSwitchInst>(PredBB->getTerminator())` should be fine.


================
Comment at: llvm/test/Transforms/DivRemPairs/X86/div-rem-pairs.ll:411
+  %b.addr.0 = phi i64 [ %add, %foo ], [ %b, %if.end ]
+  %div = udiv i64 %b.addr.0, %c
+  br label %return
----------------
I don't think this tests the right case, because the udiv/urem ops are not the same. We can't have the ops be derived from a phi.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140647/new/

https://reviews.llvm.org/D140647