This is an archive of the discontinued LLVM Phabricator instance.

Differential D108842

[ARMISelLowering] avoid emitting libcalls to __mulodi4()
ClosedPublic

Authored by nickdesaulniers on Aug 27 2021, 1:49 PM.

Details

Reviewers
rengolin
craig.topper
kristof.beyls
Commits
rG5c91b98c5d45: [ARMISelLowering] avoid emitting libcalls to __mulodi4()
Summary

has_builtin(builtin_mul_overflow) returns true for 32b ARM targets,
but Clang is deferring to compiler RT when encountering long long
types. This breaks sanitizer 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 allmodconfig builds of the Linux kernel for this
target with older releases of clang.

Link: https://bugs.llvm.org/show_bug.cgi?id=28629
Link: https://github.com/ClangBuiltLinux/linux/issues/1438

Diff Detail

Event Timeline

nickdesaulniers created this revision.Aug 27 2021, 1:49 PM
Herald added subscribers: hiraditya, kristof.beyls. · View Herald TranscriptAug 27 2021, 1:49 PM
nickdesaulniers requested review of this revision.Aug 27 2021, 1:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 27 2021, 1:49 PM
Herald added a subscriber: llvm-commits. · View Herald Transcript
nickdesaulniers added reviewers: rengolin, craig.topper, kristof.beyls.Aug 27 2021, 1:50 PM
nickdesaulniers edited subscribers, added: srhines, nathanchance; removed: kristof.beyls.
craig.topper added inline comments.Aug 27 2021, 2:07 PM
llvm/lib/Target/ARM/ARMISelLowering.cpp
547–549

Can you go ahead and add MULO_I128 here as well? C code probably can't hit it assuming __int128 isn't legal on ARM, but good to block other users.

llvm/test/CodeGen/ARM/overflow-intrinsic-optimizations.ll
241

Can you run this through mem2reg?

rengolin added a comment.Aug 27 2021, 2:15 PM

Huh! This was way simpler than I thought! Does that fix the kernel issue?

Harbormaster completed remote builds in B121546: Diff 369175.Aug 27 2021, 2:20 PM
nickdesaulniers updated this revision to Diff 369188.Aug 27 2021, 2:28 PM
nickdesaulniers edited the summary of this revision. (Show Details)
  • add MULO_I128 case
llvm/lib/Target/ARM/ARMISelLowering.cpp
547–549

Done. Indeed __int128 produces an error in clang for this target.

Want me to adapt the test to cover this as well? I don't suspect this will ever change for this target, so not sure test coverage is very interesting for this input, but happy to do so if you disagree.

llvm/test/CodeGen/ARM/overflow-intrinsic-optimizations.ll
241

If I do, then the test is no longer red before this change. Perhaps there is some other way to make this test less brittle?

nickdesaulniers added a comment.Aug 27 2021, 2:28 PM
In D108842#2969957, @rengolin wrote:

Huh! This was way simpler than I thought! Does that fix the kernel issue?

Yes.

craig.topper added inline comments.Aug 27 2021, 2:36 PM
llvm/test/CodeGen/ARM/overflow-intrinsic-optimizations.ll
241

If I do, then the test is no longer red before this change. Perhaps there is some other way to make this test less brittle?

That's weird it seems to work for me. I just wanted to get rid of the allocas and associate load/stores

define void @no__mulodi4(i32 %a, i64 %b, i32* %c) {
entry:
  %0 = sext i32 %a to i64
  %1 = call { i64, i1 } @llvm.smul.with.overflow.i64(i64 %0, i64 %b)
  %2 = extractvalue { i64, i1 } %1, 1
  %3 = extractvalue { i64, i1 } %1, 0
  %4 = trunc i64 %3 to i32
  %5 = sext i32 %4 to i64
  %6 = icmp ne i64 %3, %5
  %7 = or i1 %2, %6
  store i32 %4, i32* %c, align 4
  ret void
}

This should be even better since it prevents %10 from being dead code

define zeroext i1 @no__mulodi4(i32 %a, i64 %b, i32* %c) {
entry:
  %0 = sext i32 %a to i64
  %1 = call { i64, i1 } @llvm.smul.with.overflow.i64(i64 %0, i64 %b)
  %2 = extractvalue { i64, i1 } %1, 1
  %3 = extractvalue { i64, i1 } %1, 0
  %4 = trunc i64 %3 to i32
  %5 = sext i32 %4 to i64
  %6 = icmp ne i64 %3, %5
  %7 = or i1 %2, %6
  store i32 %4, i32* %c, align 4
  ret i1 %7
}
nickdesaulniers updated this revision to Diff 369193.Aug 27 2021, 2:41 PM
nickdesaulniers marked 2 inline comments as done.
  • mem2reg (PEBKAC)
craig.topper added inline comments.Aug 27 2021, 2:42 PM
llvm/test/CodeGen/ARM/overflow-intrinsic-optimizations.ll
241

Err that should have said %7. It was %10 before mem2reg.

nickdesaulniers updated this revision to Diff 369198.Aug 27 2021, 2:55 PM
  • use %7
nickdesaulniers marked an inline comment as done.Aug 27 2021, 2:56 PM
rengolin accepted this revision.Aug 27 2021, 3:03 PM

LGTM, thanks!

This revision is now accepted and ready to land.Aug 27 2021, 3:03 PM
This revision was landed with ongoing or failed builds.Aug 27 2021, 3:15 PM
Closed by commit rG5c91b98c5d45: [ARMISelLowering] avoid emitting libcalls to __mulodi4() (authored by nickdesaulniers). · Explain Why
This revision was automatically updated to reflect the committed changes.
nickdesaulniers added a commit: rG5c91b98c5d45: [ARMISelLowering] avoid emitting libcalls to __mulodi4().
Harbormaster completed remote builds in B121559: Diff 369198.Aug 27 2021, 3:42 PM
nickdesaulniers mentioned this in D108926: [MipsISelLowering] avoid emitting libcalls to __multi3.Aug 30 2021, 10:40 AM
nickdesaulniers mentioned this in D108928: [X86ISelLowering] avoid emitting libcalls to __mulodi4().Aug 30 2021, 10:56 AM
nickdesaulniers mentioned this in D108936: [PPCISelLowering] avoid emitting libcalls to __mulodi4().Aug 30 2021, 12:21 PM
nickdesaulniers mentioned this in D108939: [RISCVISelLowering] avoid emitting libcalls to __mulodi4() and __multi3().Aug 30 2021, 12:53 PM
nickdesaulniers mentioned this in rGd8b6ae072d77: [PPCISelLowering] avoid emitting libcalls to __mulodi4().Aug 31 2021, 11:10 AM
nickdesaulniers mentioned this in rGe9b3f2573090: [RISCVISelLowering] avoid emitting libcalls to __mulodi4() and __multi3().Aug 31 2021, 11:24 AM
nickdesaulniers mentioned this in rG6860b136b9e1: [MipsISelLowering] avoid emitting libcalls to __multi3.Sep 2 2021, 10:41 AM
nickdesaulniers mentioned this in rGd0eeb64be584: [X86ISelLowering] avoid emitting libcalls to __mulodi4().Sep 7 2021, 10:46 AM
nickdesaulniers mentioned this in D112750: [SparcISelLowering] avoid emitting libcalls to __muloti4 and __mulodi4.Oct 28 2021, 1:00 PM
nickdesaulniers mentioned this in rG39e5dd113f5c: [SparcISelLowering] avoid emitting libcalls to __muloti4 and __mulodi4.Oct 29 2021, 1:14 PM
aaronpuchert mentioned this in D122090: [PPCISelLowering] Avoid emitting calls to __multi3, __muloti4.Mar 19 2022, 4:57 PM
sebastianpoeplau mentioned this in D141836: [AArch64] Disable __muloti4 libcalls for AArch64.Jan 16 2023, 3:02 AM

Revision Contents

PathSize
llvm/
lib/
Target/
ARM/
2 lines
test/
CodeGen/
ARM/
17 lines

Diff 369204

llvm/lib/Target/ARM/ARMISelLowering.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 538 Lines • ▼ Show 20 Lines if (Subtarget->isThumb() && Subtarget->hasVFP2Base() &&
} }
} }
} }
// These libcalls are not available in 32-bit. // These libcalls are not available in 32-bit.
setLibcallName(RTLIB::SHL_I128, nullptr); setLibcallName(RTLIB::SHL_I128, nullptr);
setLibcallName(RTLIB::SRL_I128, nullptr); setLibcallName(RTLIB::SRL_I128, nullptr);
setLibcallName(RTLIB::SRA_I128, nullptr); setLibcallName(RTLIB::SRA_I128, nullptr);
setLibcallName(RTLIB::MUL_I128, nullptr); setLibcallName(RTLIB::MUL_I128, nullptr);
setLibcallName(RTLIB::MULO_I64, nullptr);
setLibcallName(RTLIB::MULO_I128, nullptr);
craig.topperUnsubmitted
Not Done
ReplyInline Actions

Can you go ahead and add MULO_I128 here as well? C code probably can't hit it assuming __int128 isn't legal on ARM, but good to block other users.

craig.topper: Can you go ahead and add MULO_I128 here as well? C code probably can't hit it assuming __int128…
nickdesaulniersAuthorUnsubmitted
Not Done
ReplyInline Actions

Done. Indeed __int128 produces an error in clang for this target.

Want me to adapt the test to cover this as well? I don't suspect this will ever change for this target, so not sure test coverage is very interesting for this input, but happy to do so if you disagree.

nickdesaulniers: Done. Indeed __int128 produces an error in clang for this target. Want me to adapt the test to…
// RTLIB // RTLIB
if (Subtarget->isAAPCS_ABI() && if (Subtarget->isAAPCS_ABI() &&
(Subtarget->isTargetAEABI() || Subtarget->isTargetGNUAEABI() || (Subtarget->isTargetAEABI() || Subtarget->isTargetGNUAEABI() ||
Subtarget->isTargetMuslAEABI() || Subtarget->isTargetAndroid())) { Subtarget->isTargetMuslAEABI() || Subtarget->isTargetAndroid())) {
static const struct { static const struct {
const RTLIB::Libcall Op; const RTLIB::Libcall Op;
const char * const Name; const char * const Name;
▲ Show 20 Lines • Show All 20,472 Lines • Show Last 20 Lines

llvm/test/CodeGen/ARM/overflow-intrinsic-optimizations.ll

Show First 20 Lines • Show All 228 Lines • ▼ Show 20 Lines
while.end: while.end:
%n.addr.0.lcssa = phi i32 [ 0, %entry ], [ 0, %while.cond ], [ %dec9.in, %land.rhs ] %n.addr.0.lcssa = phi i32 [ 0, %entry ], [ 0, %while.cond ], [ %dec9.in, %land.rhs ]
%cmp2 = icmp slt i32 %n.addr.0.lcssa, 1 %cmp2 = icmp slt i32 %n.addr.0.lcssa, 1
%conv = zext i1 %cmp2 to i32 %conv = zext i1 %cmp2 to i32
ret i32 %conv ret i32 %conv
} }
define i1 @no__mulodi4(i32 %a, i64 %b, i32* %c) {
; CHECK-LABEL: no__mulodi4
; CHECK-NOT: bl __mulodi4
entry:
%0 = sext i32 %a to i64
craig.topperUnsubmitted
Done
ReplyInline Actions

Can you run this through mem2reg?

craig.topper: Can you run this through mem2reg?
nickdesaulniersAuthorUnsubmitted
Done
ReplyInline Actions

If I do, then the test is no longer red before this change. Perhaps there is some other way to make this test less brittle?

nickdesaulniers: If I do, then the test is no longer red before this change. Perhaps there is some other way to…
craig.topperUnsubmitted
Done
ReplyInline Actions

If I do, then the test is no longer red before this change. Perhaps there is some other way to make this test less brittle?

That's weird it seems to work for me. I just wanted to get rid of the allocas and associate load/stores

define void @no__mulodi4(i32 %a, i64 %b, i32* %c) {
entry:
  %0 = sext i32 %a to i64
  %1 = call { i64, i1 } @llvm.smul.with.overflow.i64(i64 %0, i64 %b)
  %2 = extractvalue { i64, i1 } %1, 1
  %3 = extractvalue { i64, i1 } %1, 0
  %4 = trunc i64 %3 to i32
  %5 = sext i32 %4 to i64
  %6 = icmp ne i64 %3, %5
  %7 = or i1 %2, %6
  store i32 %4, i32* %c, align 4
  ret void
}

This should be even better since it prevents %10 from being dead code

define zeroext i1 @no__mulodi4(i32 %a, i64 %b, i32* %c) {
entry:
  %0 = sext i32 %a to i64
  %1 = call { i64, i1 } @llvm.smul.with.overflow.i64(i64 %0, i64 %b)
  %2 = extractvalue { i64, i1 } %1, 1
  %3 = extractvalue { i64, i1 } %1, 0
  %4 = trunc i64 %3 to i32
  %5 = sext i32 %4 to i64
  %6 = icmp ne i64 %3, %5
  %7 = or i1 %2, %6
  store i32 %4, i32* %c, align 4
  ret i1 %7
}
craig.topper: > If I do, then the test is no longer red before this change. Perhaps there is some other way…
craig.topperUnsubmitted
Done
ReplyInline Actions

Err that should have said %7. It was %10 before mem2reg.

craig.topper: Err that should have said %7. It was %10 before mem2reg.
%1 = call { i64, i1 } @llvm.smul.with.overflow.i64(i64 %0, i64 %b)
%2 = extractvalue { i64, i1 } %1, 1
%3 = extractvalue { i64, i1 } %1, 0
%4 = trunc i64 %3 to i32
%5 = sext i32 %4 to i64
%6 = icmp ne i64 %3, %5
%7 = or i1 %2, %6
store i32 %4, i32* %c, align 4
ret i1 %7
}
declare void @llvm.trap() #2 declare void @llvm.trap() #2
declare { i32, i1 } @llvm.sadd.with.overflow.i32(i32, i32) #1 declare { i32, i1 } @llvm.sadd.with.overflow.i32(i32, i32) #1
declare { i32, i1 } @llvm.uadd.with.overflow.i32(i32, i32) #1 declare { i32, i1 } @llvm.uadd.with.overflow.i32(i32, i32) #1
declare { i32, i1 } @llvm.ssub.with.overflow.i32(i32, i32) #1 declare { i32, i1 } @llvm.ssub.with.overflow.i32(i32, i32) #1
declare { i32, i1 } @llvm.usub.with.overflow.i32(i32, i32) #1 declare { i32, i1 } @llvm.usub.with.overflow.i32(i32, i32) #1
declare { i32, i1 } @llvm.smul.with.overflow.i32(i32, i32) #1 declare { i32, i1 } @llvm.smul.with.overflow.i32(i32, i32) #1
declare { i32, i1 } @llvm.umul.with.overflow.i32(i32, i32) #1 declare { i32, i1 } @llvm.umul.with.overflow.i32(i32, i32) #1
declare { i64, i1 } @llvm.smul.with.overflow.i64(i64, i64)