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

[Martin Sebor via Phabricator via llvm-commits]

msebor added inline comments.


================
Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:1018
+    return (ParamTy->isIntegerTy() && ParamTy == FTy.getParamType(2) &&
+            ParamTy->getIntegerBitWidth() >= 16);
+  }
----------------
nikic wrote:
> Any reason why this one branch checks bitwidth >= 16, but we accept any for the others?
Probably either because I forgot to duplicate it in the others, or because I forgot to remove if from here when I remembered that none of the APIs that take `long` (or `long long`) is checked for its size because the middle end doesn't expose it (I noted it in [[ https://reviews.llvm.org/D129915#inline-1259737 | this comment ]] on D129915).

Beside this, none of these checks (including `IntX`) also test for the size being a power of 2.  They all should be added, but rather than doing it piecemeal, it would be better handled comprehensively in a change of its own.


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


================
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));
----------------
nikic wrote:
> Arg0Idx -> LHSIdx or NumIdx or something? It's confusing that Arg0 and Arg0Idx don't refer to the same argument.
`Ag0Idx` corresponds to `Arg0`.  There's no `LHS` in the code, so I think the name is just fine the way it is.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:2720
+    // Fold div(0, X) to { 0, 0 } for any X.
+    createDivRetVal(CI, 0, 0, B);
+
----------------
nikic wrote:
> Missing return?
Whoops!  There sure is!  Thank you!


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:2732
+  int64_t INum = NumC->getSExtValue();
+  int64_t IDen = DenC->getSExtValue();
+
----------------
nikic wrote:
> 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.)
See my comment about `Long` (and `LLong`) checking above.

Since no target (that I know of) supports these functions with an argument wider than 64 bits, declaring and calling it with one would be undefined, so any slicing in that case would be fine.

I agree that (eventually) the TLI checking should enforce the limit(s) for all APIs that take `long` (and `long long`, and `intmax_t`), so using `APInt` for what in practice is never bigger than `int64_t` now would be unnecessary and inefficient (and inconsistent with what's used in most other places I've seen).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129396