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

Fri Jul 8 15:27:09 PDT 2022

msebor added a comment.

Thanks for the pointer to D129396 <https://reviews.llvm.org/D129396>!

I didn't expect calling convention differences to be visible this early in the middle end (they're not in GCC).  That seems unfortunate, but at least I learned something new.  That said, on some of the targets mentioned in D129396 <https://reviews.llvm.org/D129396> such as arm or i386, `LibCallSimplifier::optimizeDiv` isn't invoked for calls to `div` because they pass the function three arguments rather than the expected two (a pointer to `struct div_t` to fill with the result, followed by the two expected integers), and so those calls are not recognized (`TargetLibraryInfo::getLibFunc()` returns `false`).  So while those targets don't benefit from the change I wouldn't expect them to be adversely affected by it.  Unless this isn't the only other calling convention, presumably support for it could be added in a follow-up.  If there are others then adding a built-in for it might be an option to explore.  Or, if `div` et al. aren't worth the trouble, maybe I should just add tests to catch this futile attempt for an enhancement early in the future and keep others from wasting time with it.

The patch does cause a couple of test failures that I neglected to mention:

  Failed Tests (2):
    LLVM :: tools/llvm-tli-checker/ps4-tli-check.yaml
    LLVM-Unit :: Analysis/./AnalysisTests/TargetLibraryInfoTest/ValidProto

They don't look like they're obviously due to ABI differences but I haven't looked into them in any detail to fully understand them yet.  My guess is that not all targets (e.g., PlayStation 4?) support all four functions and that the patch is missing some conditionals to avoid assuming all four have the expected semantics.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129396