[clang] [clang][codegen] Don't mark "int" TBAA on FP libcalls with indirect args (PR #108853)

[Zahira Ammarguellat via cfe-commits]

zahiraam wrote:

> > > > > > How does this interact with #107598?
> > > > > 
> > > > > 
> > > > > Though this also changes things to ensure when TBAA data is set, it's always set on the call.
> > > > 
> > > > 
> > > > Wasn't already doing that? (setting the TBAA on the call?).
> > > 
> > > 
> > > It was setting it on `cast<llvm::Instruction>(Call.getScalarVal())` not necessarily the call (which you can get via an output on `EmitCall()`). At least in this case that meant it was putting the TBAA metadata on the `load x86_fp80` after the call. I'm not sure if there's other cases where something similar could happen.
> > 
> > 
> > > > > > How does this interact with #107598?
> > > > > 
> > > > > 
> > > > > Though this also changes things to ensure when TBAA data is set, it's always set on the call.
> > > > 
> > > > 
> > > > Wasn't already doing that? (setting the TBAA on the call?).
> > > 
> > > 
> > > It was setting it on `cast<llvm::Instruction>(Call.getScalarVal())` not necessarily the call (which you can get via an output on `EmitCall()`). At least in this case that meant it was putting the TBAA metadata on the `load x86_fp80` after the call. I'm not sure if there's other cases where something similar could happen.
> > 
> > 
> > Without this patch and without (#107598) the function `pow` doesn't generate `int TBAA` info on the call, but it does on a call to `cargl` with `-triple aarch64-unknown-unknown`.
> > `# | %call = tail call fp128 @cargl([2 x fp128] noundef alignstack(16) undef) #1, !tbaa !2`
> 
> I'm not sure I follow, but the issue I spotted was "int" TBAA metadata was being set on the load following the `pow` call, but it has the same root cause as the `cargl` issue.
> 
> See the diff from the first commit:
> 
> ```diff
> diff --git a/clang/test/CodeGen/math-libcalls-tbaa-indirect-args.c b/clang/test/CodeGen/math-libcalls-tbaa-indirect-args.c
> index dd013dcc8b3ca8..56c6b3ec00bc7e 100644
> --- a/clang/test/CodeGen/math-libcalls-tbaa-indirect-args.c
> +++ b/clang/test/CodeGen/math-libcalls-tbaa-indirect-args.c
> @@ -19,7 +19,7 @@ long double powl(long double a, long double b);
>  // CHECK-NEXT:    call void @llvm.lifetime.start.p0(i64 16, ptr nonnull [[BYVAL_TEMP1]]) #[[ATTR3]]
>  // CHECK-NEXT:    store x86_fp80 0xK40008000000000000000, ptr [[BYVAL_TEMP1]], align 16, !tbaa [[TBAA3]]
>  // CHECK-NEXT:    call void @powl(ptr dead_on_unwind nonnull writable sret(x86_fp80) align 16 [[TMP]], ptr noundef nonnull [[BYVAL_TEMP]], ptr noundef nonnull [[BYVAL_TEMP1]]) #[[ATTR3]]
> -// CHECK-NEXT:    [[TMP0:%.*]] = load x86_fp80, ptr [[TMP]], align 16, !tbaa [[TBAA7:![0-9]+]]
> +// CHECK-NEXT:    [[TMP0:%.*]] = load x86_fp80, ptr [[TMP]], align 16, !tbaa [[TBAA3]]
>  // CHECK-NEXT:    call void @llvm.lifetime.end.p0(i64 16, ptr nonnull [[BYVAL_TEMP]]) #[[ATTR3]]
>  // CHECK-NEXT:    call void @llvm.lifetime.end.p0(i64 16, ptr nonnull [[BYVAL_TEMP1]]) #[[ATTR3]]
>  // CHECK-NEXT:    store x86_fp80 [[TMP0]], ptr [[AGG_RESULT]], align 16, !tbaa [[TBAA3]]
> @@ -33,6 +33,4 @@ long double test_powl() {
>  // CHECK: [[META4]] = !{!"long double", [[META5:![0-9]+]], i64 0}
>  // CHECK: [[META5]] = !{!"omnipotent char", [[META6:![0-9]+]], i64 0}
>  // CHECK: [[META6]] = !{!"Simple C/C++ TBAA"}
> -// CHECK: [[TBAA7]] = !{[[META8:![0-9]+]], [[META8]], i64 0}
> -// CHECK: [[META8]] = !{!"int", [[META5]], i64 0}
>  //.
> ```
> 
> > but it does on a call to cargl with `-triple aarch64-unknown-unknown`.
> > `> `# |   %call = tail call fp128 @cargl([2 x fp128] noundef alignstack(16) undef) #1, !tbaa !2`
> 
> That looks okay though? It's not passing or returning values via pointers, so it should be safe to set the "int" TBAA (which indicates the only pointer it could read is errno).

Not really! `int TBAA` in in our downstream compiler is interpreted as describing the function arguments (they are not int) and the `load/store` of the argument before the library call are begin eliminated which result in unexpected behavior.
I am not objecting to anything; I am just wondering what difference it makes to have the TBAA attached to the call instead of how it is now.

https://github.com/llvm/llvm-project/pull/108853