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

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:3400
+        Callee->getAttributes().hasParamAttr(i, Attribute::ByVal))
+      return false; // Cannot transform to byval.
+
----------------
glandium wrote:
> 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.
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.


================
Comment at: llvm/test/Transforms/InstCombine/cast-to-byval.ll:3
+; Check that function calls involving conversion to byval aren't transformed.
+; RUN: opt < %s -passes=instcombine -mtriple=x86_64-unknown-unknown -S | FileCheck %s
+
----------------
Drop the mtriple here -- that shouldn't be needed and would require a `REQUIRES` to avoid breakage when the target is not built.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135738