[InstCombine] Bail out of casting calls when a conversion from/to byval is involved.

[glandium]

Summary

Fixes #58307

[arsenm]

Needs test case

[glandium]

Added testcase.

[glandium]

Refreshed test case to match what's in the issue.

[arsenm]

llvm/test/LTO/X86/cast-to-byval.ll

3 ↗

(On Diff #466999)

Why does this test involve LTO or multiple modules? Why can't you just run instcombine?

[glandium]

llvm/test/LTO/X86/cast-to-byval.ll

3 ↗

(On Diff #466999)

Because I have no idea how to reproduce without LTO. I failed with a bitcast of the function.

[glandium]

llvm/test/LTO/X86/cast-to-byval.ll

3 ↗

(On Diff #466999)

Scrap that, just calling without a bitcast works.

[glandium]

InstCombine test.

[glandium]

Adjustment to the comment.

[glandium]

Added missing datalayout and target.

(Sorry for the update noise)

[efriedma]

I don't think your patch completely solves the issue you're describing. What happens if you have a struct where the only member is a pointer? clang will lower it to "ptr", rust will lower it to "ptr byval", and whatever LTO side-effects you're seeing will happen without this instcombine transform ever getting involved.

Not sure what the right solution looks like here; the current CallBase::getCalledFunction() isn't really designed to consider this case.

[glandium]

It looks like it's not a problem:

cat <<EOF | opt --O3 -S
target datalayout = "e-m:e-p:32:32-p270:32:32-p271:32:32-p272:64:64-f64:32:64-f80:32-n8:16:32-S128"
target triple = "i686-unknown-linux-gnu"

define ptr @bar(ptr %0) {
  %2 = tail call ptr @foo(ptr %0)
  ret ptr %2
}

%Foo = type { ptr }
define ptr @foo (ptr byval(%Foo) %foo) {
  %1 = load ptr, ptr %foo, align 4
  ret ptr %1
}
EOF
; ModuleID = '<stdin>'
source_filename = "<stdin>"
target datalayout = "e-m:e-p:32:32-p270:32:32-p271:32:32-p272:64:64-f64:32:64-f80:32-n8:16:32-S128"
target triple = "i686-unknown-linux-gnu"

%Foo = type { ptr }

; Function Attrs: argmemonly mustprogress nofree norecurse nosync nounwind readonly willreturn
define ptr @bar(ptr nocapture readonly %0) local_unnamed_addr #0 {
  %2 = load ptr, ptr %0, align 4
  ret ptr %2
}

; Function Attrs: argmemonly mustprogress nofree norecurse nosync nounwind readonly willreturn
define ptr @foo(ptr nocapture readonly byval(%Foo) %foo) local_unnamed_addr #0 {
  %1 = load ptr, ptr %foo, align 4
  ret ptr %1
}

attributes #0 = { argmemonly mustprogress nofree norecurse nosync nounwind readonly willreturn }

(that's without the patch applied, btw ; with or without the patch makes no difference in this case)

[nikic]

Example for the case @efriedma mentions: https://llvm.godbolt.org/z/oPzWv9sr4

I believe this bug is on the rustc side and needs to be fixed there. There is an ABI mismatch on the IR level here, and I don't think we can ask IR transforms to reason about post-legalization ABI changes. We all know that LLVM's current way of dealing with platform ABIs is problematic, but we have to deal with what we have right now, and that requires all frontends to emit the same IR signature.

[efriedma]

IR optimizations accommodate mismatches to some extent; if the types of the caller and the callee don't match, we treat the call as opaque, which effectively makes the call work if the actual registers match. But that doesn't detect ABI-modifying attributes.

It might be possible to adjust optimizations to handle this conservatively without any deep infrastructure changes. Basically, treat calls as opaque if there are significant ABI attributes that don't match... but I'm not sure what exactly that check would include, or how we make it fast.

---

That said, I don't have any objection to this patch... I mean, it can't really do any harm. I just want to be sure we're on the same page about what it does, and does not accomplish.

[arsenm]

llvm/test/Transforms/InstCombine/cast-to-byval.ll

4–5 ↗

(On Diff #467004)

Shouldn't need this

[nikic]

In D135738#3853262, @efriedma wrote:

IR optimizations accommodate mismatches to some extent; if the types of the caller and the callee don't match, we treat the call as opaque, which effectively makes the call work if the actual registers match. But that doesn't detect ABI-modifying attributes.

It might be possible to adjust optimizations to handle this conservatively without any deep infrastructure changes. Basically, treat calls as opaque if there are significant ABI attributes that don't match... but I'm not sure what exactly that check would include, or how we make it fast.

---

That said, I don't have any objection to this patch... I mean, it can't really do any harm. I just want to be sure we're on the same page about what it does, and does not accomplish.

My problem with this patch is that it does not address the problem in a principled way: It works around one specific instance of this problem, while retaining it in many other places. As you already pointed out, inlining for the struct containing a pointer case is broken. From a quick try, the same is true for argument promotion (https://llvm.godbolt.org/z/8We7dvYvT), we mix up the levels of indirection. The same is probably true for other IPO transforms as well. There's probably also issues with attribute interpretation that are much more wide-spread (e.g. does a nonnull refer to the byval pointer or its payload?)

If we wanted to support this, I think we would have to make an effort to support it more broadly. And I personally don't think this is justified, especially as this will need to be fixed on the rustc side anyway (if nothing else then to enable cross-language inlining after these changes).

[glandium]

llvm/test/Transforms/InstCombine/cast-to-byval.ll

4–5 ↗

(On Diff #467004)

The test passes with the unpatched code without this.

[glandium]

From a quick try, the same is true for argument promotion (https://llvm.godbolt.org/z/8We7dvYvT), we mix up the levels of indirection.

This one doesn't happen without internal, which wouldn't happen in a real cross-module case, would it?

[glandium]

Example for the case @efriedma mentions: https://llvm.godbolt.org/z/oPzWv9sr4

This one is interesting because apart if you had a struct Foo { struct Foo* }, it seems to be a problem created by opaque pointers.

[nikic]

In D135738#3854021, @glandium wrote:

From a quick try, the same is true for argument promotion (https://llvm.godbolt.org/z/8We7dvYvT), we mix up the levels of indirection.

This one doesn't happen without internal, which wouldn't happen in a real cross-module case, would it?

It can happen after fat LTO internalization. For thin LTO, I don't think it's possible right now (though I believe someone is working on supporting ThinLTO arg promotion, so that may not hold up in the future...)

[glandium]

Would you reconsider this patch? Sure, it doesn't address the issue generally, but it does fix the cases that were actively broken by 6c8adc5.

FWIW, https://llvm.godbolt.org/z/oPzWv9sr4 was already miscompiled with LLVM 14. As for https://llvm.godbolt.org/z/8We7dvYvT, it crashes LLVM 14.

[nikic]

Okay, I looked a bit closer into what clang is doing here with the intention of implementing the same in rustc. This turns out to be more complicated than I originally thought (some notes in https://github.com/rust-lang/rust/issues/102174#issuecomment-1285502667). I believe this would require an entirely new PassMode, which can model the unpacked struct plus a possibly needed inreg padding register.

With that in mind, I'm okay with landing this mitigation in the meantime, especially as this does not lose any practical transformation power.

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

3400

Do we need to prevent the reverse direction as well? I.e. call-site has byval, function doesn't.

llvm/test/Transforms/InstCombine/cast-to-byval.ll

2 ↗

(On Diff #467004)

Use update_test_checks.py.

4–5 ↗

(On Diff #467004)

You need to change to an i64 argument when dropping the data layout, so it matches the ptr size.

[glandium]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

3400

In theory, I guess we do. At least from the "better safe than sorry" perspective. But in practice it doesn't seem to be a problem: no conversion from byval to integer is attempted, and for pointers, the call is not changed in the first place. https://llvm.godbolt.org/z/5ov489MxP

[glandium]

llvm/test/Transforms/InstCombine/cast-to-byval.ll

2 ↗

(On Diff #467004)

Interestingly, the CHECKs it produces fail...

[nikic]

llvm/test/Transforms/InstCombine/cast-to-byval.ll

2 ↗

(On Diff #467004)

At a guess, you need to reorder the foo and bar functions or pass --function-signature.

[glandium]

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

3400

Preventing conversions from byval actually breaks Transforms/InstCombine/byval.ll. This is what update_test_checks.py would update:

--- a/llvm/test/Transforms/InstCombine/byval.ll
+++ b/llvm/test/Transforms/InstCombine/byval.ll
@@ -7,8 +7,7 @@ declare void @add_byval_callee_2(double* byval(double))
 
 define void @add_byval(i64* %in) {
 ; CHECK-LABEL: @add_byval(
-; CHECK-NEXT:    [[TMP1:%.*]] = bitcast i64* [[IN:%.*]] to double*
-; CHECK-NEXT:    call void @add_byval_callee(double* byval(double) [[TMP1]])
+; CHECK-NEXT:    call void bitcast (void (double*)* @add_byval_callee to void (i64*)*)(i64* byval(i64) [[IN:%.*]])
 ; CHECK-NEXT:    ret void
 ;
   %tmp = bitcast void (double*)* @add_byval_callee to void (i64*)*

This change however doesn't feel wrong. Something seems off with the original test.

llvm/test/Transforms/InstCombine/cast-to-byval.ll

2 ↗

(On Diff #467004)

Reordering made it work. Thanks.

[glandium]

Removed datalayout/Ran update_test_checks

[nikic]

LGTM

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

3400

That change would be fine (in the sense that it is in line with the restriction you're introducing here), though it looks like this ultimately doesn't matter with opaque pointers.

llvm/test/Transforms/InstCombine/cast-to-byval.ll

3 ↗

(On Diff #469365)

Drop the mtriple here -- that shouldn't be needed and would require a REQUIRES to avoid breakage when the target is not built.

[glandium]

llvm/test/Transforms/InstCombine/cast-to-byval.ll

3 ↗

(On Diff #469365)

does opt default to 64-bits on 32-bits hosts or does running tests on 32-bits not supported?

[nikic]

llvm/test/Transforms/InstCombine/cast-to-byval.ll

3 ↗

(On Diff #469365)

Opt uses a 64-bit data layout by default, independent of host.

[glandium]

Removed -mtriple ; Restricted conversions from byval as well.

[glandium]

@nikic can you push this for me?

[sylvestre.ledru]

Just did!

llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp

for (unsigned i = 0, e = NumCommonArgs; i != e; ++i, ++AI) {
 
     if (Call.isInAllocaArgument(i) ||
         CallerPAL.hasParamAttr(i, Attribute::Preallocated))
       return false; // Cannot transform to and from inalloca/preallocated.
 
     if (CallerPAL.hasParamAttr(i, Attribute::SwiftError))
       return false;
 
+    if (CallerPAL.hasParamAttr(i, Attribute::ByVal) !=
+        Callee->getAttributes().hasParamAttr(i, Attribute::ByVal))
+      return false; // Cannot transform to or from byval.
+
     // If the parameter is passed as a byval argument, then we have to have a
     // sized type and the sized type has to have the same size as the old type.
     if (ParamTy != ActTy && CallerPAL.hasParamAttr(i, Attribute::ByVal)) {
       PointerType *ParamPTy = dyn_cast<PointerType>(ParamTy);
       if (!ParamPTy)
         return false;
 
       if (!ParamPTy->isOpaque()) {

llvm/test/Transforms/InstCombine/byval.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -S -passes=instcombine %s | FileCheck %s
 
 declare void @add_byval_callee(double*)
 
 declare void @add_byval_callee_2(double* byval(double))
 
 define void @add_byval(i64* %in) {
 ; CHECK-LABEL: @add_byval(
-; CHECK-NEXT:    [[TMP1:%.*]] = bitcast i64* [[IN:%.*]] to double*
-; CHECK-NEXT:    call void @add_byval_callee(double* byval(double) [[TMP1]])
+; CHECK-NEXT:    call void bitcast (void (double*)* @add_byval_callee to void (i64*)*)(i64* byval(i64) [[IN:%.*]])
 ; CHECK-NEXT:    ret void
 ;
   %tmp = bitcast void (double*)* @add_byval_callee to void (i64*)*
   call void %tmp(i64* byval(i64) %in)
   ret void
 }
 
 define void @add_byval_2(i64* %in) {

llvm/test/Transforms/InstCombine/cast-byval.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; Check that function calls involving conversion from/to byval aren't transformed.
+; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+
+%Foo = type { i64 }
+define i64 @foo (ptr byval(%Foo) %foo) {
+; CHECK-LABEL: @foo(
+; CHECK-NEXT:    [[TMP1:%.*]] = load i64, ptr [[FOO:%.*]], align 4
+; CHECK-NEXT:    ret i64 [[TMP1]]
+;
+  %1 = load i64, ptr %foo, align 4
+  ret i64 %1
+}
+
+define i64 @bar(i64 %0) {
+; CHECK-LABEL: @bar(
+; CHECK-NEXT:    [[TMP2:%.*]] = tail call i64 @foo(i64 [[TMP0:%.*]])
+; CHECK-NEXT:    ret i64 [[TMP2]]
+;
+  %2 = tail call i64 @foo(i64 %0)
+  ret i64 %2
+}
+
+define i64 @qux(ptr byval(%Foo) %qux) {
+; CHECK-LABEL: @qux(
+; CHECK-NEXT:    [[TMP1:%.*]] = tail call i64 @bar(ptr nonnull byval([[FOO:%.*]]) [[QUX:%.*]])
+; CHECK-NEXT:    ret i64 [[TMP1]]
+;
+  %1 = tail call i64 @bar(ptr byval(%Foo) %qux)
+  ret i64 %1
+}