Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Functions merging doesn't respect TBAA metadata on store instructions #123462

Open
Panzerschrek opened this issue Jan 18, 2025 · 1 comment
Open

Comments

@Panzerschrek
Copy link

See

if (const StoreInst *SI = dyn_cast<StoreInst>(L)) {

Metadata for store instructions (including TBAA) isn't compared in function comparator. This may lead to merging functions with different TBAA metadata. This means breaking further optimizations based on TBAA, including removing loads and stores wrongly assumed to be redundant and optimized-out.

Example generated by my language compiler:

%"1A" = type { i64 }
%"1B" = type { i64 }

; Function Attrs: nounwind
define private void @_Z6BStoreR1By(ptr noalias nonnull dereferenceable(8) %_arg_b, i64 %_arg_x) unnamed_addr #0 {
allocations:
  %x = alloca i64, align 8
  %0 = getelementptr inbounds %"1B", ptr %_arg_b, i32 0, i32 0
  br label %func_code

func_code:                                        ; preds = %allocations
  store i64 %_arg_x, ptr %x, align 8, !tbaa !7
  %1 = load i64, ptr %x, align 8, !tbaa !7
  store i64 %1, ptr %0, align 8, !tbaa !7
  ret void
}

; Function Attrs: nounwind
define private void @_Z6AStoreR1Ax(ptr noalias nonnull dereferenceable(8) %_arg_a, i64 %_arg_x) unnamed_addr #0 {
allocations:
  %x = alloca i64, align 8
  %0 = getelementptr inbounds %"1A", ptr %_arg_a, i32 0, i32 0
  br label %func_code

func_code:                                        ; preds = %allocations
  store i64 %_arg_x, ptr %x, align 8, !tbaa !0
  %1 = load i64, ptr %x, align 8, !tbaa !0
  store i64 %1, ptr %0, align 8, !tbaa !0
  ret void
}

; Function Attrs: nounwind
define hidden ptr @GetAStore() unnamed_addr #0 {
allocations:
  br label %func_code

func_code:                                        ; preds = %allocations
  ret ptr @_Z6AStoreR1Ax
}

; Function Attrs: nounwind
define hidden ptr @GetBStore() unnamed_addr #0 {
allocations:
  br label %func_code

func_code:                                        ; preds = %allocations
  ret ptr @_Z6BStoreR1By
}

attributes #0 = { nounwind }

!0 = !{!1, !1, i64 0}
!1 = !{!"i64", !2, i64 0}
!2 = !{!"byte64", !3, i64 0}
!3 = !{!"byte32", !4, i64 0}
!4 = !{!"byte16", !5, i64 0}
!5 = !{!"byte8", !6, i64 0}
!6 = !{!"__U_tbaa_root"}
!7 = !{!8, !8, i64 0}
!8 = !{!"u64", !2, i64 0}

Functions BStore and AStore have almost identical code - they store a value in memory, but using different TBAA tags - for i64 and u64 language types (which are in my language distinct from each other).

After running optimization passes (including functions merge pass) I get following code:

; Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(argmem: write)
define private void @_Z6AStoreR1Ax(ptr noalias nocapture nonnull writeonly dereferenceable(8) %_arg_a, i64 %_arg_x) unnamed_addr #0 {
allocations:
  store i64 %_arg_x, ptr %_arg_a, align 8, !tbaa !0
  ret void
}

; Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(none)
define hidden nonnull ptr @GetAStore() unnamed_addr #1 {
allocations:
  ret ptr @_Z6AStoreR1Ax
}

; Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(none)
define hidden nonnull ptr @GetBStore() unnamed_addr #1 {
allocations:
  ret ptr @_Z6AStoreR1Ax
}

attributes #0 = { mustprogress nofree norecurse nosync nounwind willreturn memory(argmem: write) }
attributes #1 = { mustprogress nofree norecurse nosync nounwind willreturn memory(none) }

!0 = !{!1, !1, i64 0}
!1 = !{!"i64", !2, i64 0}
!2 = !{!"byte64", !3, i64 0}
!3 = !{!"byte32", !4, i64 0}
!4 = !{!"byte16", !5, i64 0}
!5 = !{!"byte8", !6, i64 0}
!6 = !{!"__U_tbaa_root"}

Two functions were merged, but this is incorrect behavior. In such example it's not a big deal, but in cases where such functions may be inlined later this may cause a bug leading to miscompilation and crashes in result binary code.

@Panzerschrek
Copy link
Author

Fixing this is trivial - cmpInstMetadata call should be used to compare not only load, but also store instructions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants