[PATCH] D135738: [InstCombine] Bail out of casting calls when a conversion to byval is involved.

[Mike Hommey via Phabricator via llvm-commits]

glandium added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:3400
+        Callee->getAttributes().hasParamAttr(i, Attribute::ByVal))
+      return false; // Cannot transform to byval.
+
----------------
glandium wrote:
> nikic wrote:
> > Do we need to prevent the reverse direction as well? I.e. call-site has byval, function doesn't.
> 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
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.


================
Comment at: llvm/test/Transforms/InstCombine/cast-to-byval.ll:2
+; Check that function calls involving conversion to byval aren't transformed.
+; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+
----------------
nikic wrote:
> glandium wrote:
> > nikic wrote:
> > > Use update_test_checks.py.
> > Interestingly, the CHECKs it produces fail...
> At a guess, you need to reorder the foo and bar functions or pass `--function-signature`.
Reordering made it work. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135738