[PATCH] D126644: [llvm/CodeGen] Add ExpandLargeDivRem pass

[Matthias Gehre via Phabricator via llvm-commits]

mgehre-amd added a comment.

Thanks for the review! I updated the PR to reflect your comments.



================
Comment at: llvm/lib/CodeGen/ExpandLargeDivRem.cpp:47
+    case Instruction::SRem: {
+      auto *IntTy = dyn_cast<IntegerType>(I.getType());
+      if (!IntTy || IntTy->getIntegerBitWidth() <= ExpandDivRemBits)
----------------
arsenm wrote:
> This won't handle vectors
True, I'm not concerned about vectors right now. I can add a TODO comment here.


================
Comment at: llvm/lib/CodeGen/ExpandLargeDivRem.cpp:84
+
+  return PreservedAnalyses::all();
+}
----------------
nikic wrote:
> At least for the new pass manager, I don't think it's legal to add new functions in a function pass. I'm not sure about the legacy pass manager.
Yes, this was one of the early comments I got on this PR, so I changed it to not insert new functions but instead insert the loop in-place.


================
Comment at: llvm/test/CodeGen/X86/urem-seteq.ll:380
-; X64-NEXT:    sete (%rax)
-; X64-NEXT:    retq
   %L10 = load i448, i448* undef, align 4
----------------
nikic wrote:
> What happened here?
This IR used to crash LLVM. I don't think that the actual assembly is relevant here, and after my changes it's much longer due to the loops that were inserted.


================
Comment at: llvm/test/Transforms/ExpandLargeDivRem/urem129.ll:13
+; CHECK-NEXT:    [[A:%.*]] = load i129, i129* [[PTR:%.*]], align 4
+; CHECK-NEXT:    [[TMP1:%.*]] = call i129 @__llvm_urem129(i129 [[A]], i129 3)
+; CHECK-NEXT:    store i129 [[TMP1]], i129* [[OUT:%.*]], align 4
----------------
arsenm wrote:
> This looks out of date?
Yes, stupid me. I'll update.


================
Comment at: llvm/test/Transforms/ExpandLargeDivRem/values129.ll:21
+;
+  %ret = call {i129, i129} @udivrem(i129 1, i129 1)
+  ret {i129, i129} %ret
----------------
arsenm wrote:
> I'm not sure cases that constant fold are the most helpful tests
I wrote this test when the implementation of the shift-subtract loop was still mine. It is verifying that
the value that is computed by that loop is the same as the value of the urem.
This is done by comparing whether constant-folding the urem directly gives the same value
as first replacing the urem by expandlargedivrem and then constant folding the result.

Then, I learned that there was existing implementation of the shift-subtract loop in llvm/lib/Transforms/Utils/IntegerDivision.cpp and switched to that.
Because Utils/IntegerDivision.cpp is already tested, I will get rid of this test here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126644