[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/Transforms/Utils/SimplifyLibCalls.cpp:2668
+    // Store the result in consecutive members of the returned struct,
+    // assuming they are laid out in the usual order: { T quot, rem; }.
+    RetTy = Callee->getParamStructRetType(0);
----------------
xbolva00 wrote:
> Strong assumption? Is this order defined by C standard?
No (as noted in the Summary and in D65457#1606877).

The implementations I reviewed (Android, Darwin, FreeBSD, Glibc, HP-UX, Illumos/Solaris, Newlib, OpenBSD, QNX, and Windows), as well as LLVM's own libc, all define the struct as it's commonly documented.  A [[ https://sourcegraph.com/search?q=context:global+%5B+%5Ct%5D*%5Bl%5D*div_t%5B+%5Ct%5D*+file:stdlib.h&patternType=regexp&case=yes | Code Search ]] query also didn't reveal anything unexpected.  But to be absolutely sure the transformation could either be parameterized on the result of a configury test or enabled only for the systems where the layout is known.  I don't know how to do the former yet but I'm open to either if someone feels it's necessary.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:2703
+
+  if (dyn_cast<VectorType>(RetTy))
+    return ConstantVector::get({QuotC, RemC});
----------------
majnemer wrote:
> nit: you can use isa here instead of dyn_cast
Right, will do, thanks!


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

https://reviews.llvm.org/D129396