This is an archive of the discontinued LLVM Phabricator instance.

[X86ISelLowering] avoid emitting libcalls to __mulodi4()

Authored by nickdesaulniers on Aug 30 2021, 10:56 AM.



Similar to D108842, D108844, and D108926.

__has_builtin(builtin_mul_overflow) returns true for 32b x86 targets,
but Clang is deferring to compiler RT when encountering long long types.
This breaks ARCH=i386 + CONFIG_BLK_DEV_NBD=y builds of the Linux kernel
that are using builtin_mul_overflow with these types for these targets.

If the semantics of __has_builtin mean "the compiler resolves these,
always" then we shouldn't conditionally emit a libcall.

This will still need to be worked around in the Linux kernel in order to
continue to support these builds of the Linux kernel for this
target with older releases of clang.


Diff Detail

Event Timeline

nickdesaulniers requested review of this revision.Aug 30 2021, 10:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2021, 10:56 AM
nickdesaulniers planned changes to this revision.Aug 30 2021, 11:08 AM

looks like I need to update CodeGen/X86/smul_fix_sat.ll and CodeGen/X86/xmulo.ll

nickdesaulniers edited the summary of this revision. (Show Details)Aug 30 2021, 11:12 AM
nickdesaulniers edited the summary of this revision. (Show Details)
  • fix up tests
lebedev.ri accepted this revision.Aug 30 2021, 11:20 AM

Yay, LGTM, but please wait for @craig.topper / @RKSimon :)

This revision is now accepted and ready to land.Aug 30 2021, 11:20 AM
RKSimon added inline comments.Aug 31 2021, 12:51 AM

Please use the script to generate checks

nickdesaulniers marked an inline comment as done.
  • run

Honestly, is a big hammer. It's going to add 84 lines to this test, which simply checks we don't call one function __mulodi4. I don't care what the resulting code is, as long as we don't call the function. I've added it as you asked, but I think it significantly hurts the readability of this test.

It's convenient for sure, but it ends up generating significant churn in the tests, and reviewers don't need to think about why a test was important or whether they're changing a test for the worst; they can just run an automated tool until the test passes; who cares what the original intent of the test ever was and whether we just steam-rolled it?

craig.topper added inline comments.Aug 31 2021, 11:49 AM

I think I agree with Nick here. The only value this test has over xmulo.ll is that the test is named no__mulodi4. The -NOT was the most straightforward way to check exactly what the intent of this test is. With the script being run on it we lose that and might as well not have it and rely on xmulo.ll


That's reasonable; in that case @RKSimon would you prefer I either:

  1. delete this newly added test, llvm/test/CodeGen/X86/overflow-intrinsic-optimizations.ll and rely on the other modified tests for coverage?
  2. revert llvm/test/CodeGen/X86/overflow-intrinsic-optimizations.ll back to 369497?

(I guess there's also 3. commit code as is...)

rengolin added inline comments.Aug 31 2021, 2:45 PM

Agreed. The original check was pretty clear to me what the test was doing.

RKSimon added inline comments.Sep 1 2021, 3:04 AM

OK, going back to the original, minimal checks is fine as long as you're not intending to add many more tests to this file.

nickdesaulniers marked 3 inline comments as done.Sep 1 2021, 10:32 AM
RKSimon accepted this revision.Sep 7 2021, 10:20 AM

LGTM (sorry for delay)

This revision was landed with ongoing or failed builds.Sep 7 2021, 10:46 AM
This revision was automatically updated to reflect the committed changes.