This is an archive of the discontinued LLVM Phabricator instance.

[ARMISelLowering] avoid emitting libcalls to __mulodi4()
ClosedPublic

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

Details

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 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
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?

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

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?

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 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 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
This revision was automatically updated to reflect the committed changes.