[PATCH] D129396: [InstCombine] Add support for div, ldiv, lldiv, and imaxdiv folding

[Nikita Popov via Phabricator via llvm-commits]

nikic added a reviewer: efriedma.
nikic added a subscriber: efriedma.
nikic added a comment.

This looks reasonable to me at a glance. Adding @efriedma for some additional feedback on this kind of very ABI dependent fold.

Given the ABI-dependent nature, it might be worthwhile to add some C tests for these functions to llvm-test-suite, so we know if some of the assumptions here break down.



================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:1018
+    return (ParamTy->isIntegerTy() && ParamTy == FTy.getParamType(2) &&
+            ParamTy->getIntegerBitWidth() >= 16);
+  }
----------------
Any reason why this one branch checks bitwidth >= 16, but we accept any for the others?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:2664
+    Function *Callee = CI->getCalledFunction();
+    if (!Callee->hasStructRetAttr())
+      return nullptr;
----------------
I would consider it slightly more elegant to call do a `!RetTy` check instead on the result of `getParamStructRetType()`, as that is the value we end up relying on.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:2670
+    RetTy = Callee->getParamStructRetType(0);
+    StructType *StructTy = dyn_cast<StructType>(RetTy);
+    Value *RetPtr = CI->getOperand(0);
----------------
Shouldn't we check that `StructTy` has the expected form? The libfunc verification doesn't check how the sret type looks like.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:2678
+    if (ArrayTy->getNumElements() != 2)
+      return nullptr;
+    return ConstantArray::get(ArrayTy, {QuotC, RemC});
----------------
This could be an assert (to make sure the TLI check is in sync).


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:2712
+  Value *Arg0 = CI->getArgOperand(0);
+  int Arg0Idx = Arg0->getType()->isPointerTy();
+  ConstantInt *NumC = dyn_cast<ConstantInt>(CI->getArgOperand(Arg0Idx));
----------------
Arg0Idx -> LHSIdx or NumIdx or something? It's confusing that Arg0 and Arg0Idx don't refer to the same argument.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:2720
+    // Fold div(0, X) to { 0, 0 } for any X.
+    createDivRetVal(CI, 0, 0, B);
+
----------------
Missing return?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:2732
+  int64_t INum = NumC->getSExtValue();
+  int64_t IDen = DenC->getSExtValue();
+
----------------
If I'm seeing this right, we don't guard against > 64-bit integers anywhere (should probably be part of the libcall check? Or else, this code could just keep working on APInt and not care about bit widths -- that would be more idiomatic.)


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

https://reviews.llvm.org/D129396