Page MenuHomePhabricator

[clang][RISCV] Fix ABI handling of empty structs with hard FP calling conventions in C++
AcceptedPublic

Authored by asb on Jan 22 2023, 10:09 PM.

Details

Summary

As reported in https://github.com/llvm/llvm-project/issues/58929,
Clang's handling of empty structs in the case of small structs that may
be eligible to be passed using the hard FP calling convention doesn't
match g++. In general, C++ record fields are never empty unless
[[no_unique_address]] is used, but the RISC-V FP ABI overrides this.

After this patch, fields of structs that contain empty records will be
ignored, even in C++, when considering eligibility for the FP calling
convention ('flattening'). It isn't explicitly noted in the RISC-V
psABI, but arrays of empty records will disqualify a struct for
consideration of using the FP calling convention in g++. This patch
matches that behaviour. The psABI issue
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/358 seeks
to clarify this.

Diff Detail

Unit TestsFailed

TimeTest
3,610 mslibcxx CI Modules > llvm-libc++-shared-cfg-in.libcxx/algorithms/specialized_algorithms/special_mem_concepts::nothrow_sentinel_for.compile.pass.cpp
Script: -- : 'COMPILED WITH'; /home/libcxx-builder/.buildkite-agent/builds/aaa2661d6269-1/llvm-project/libcxx-ci/install/bin/clang++ /home/libcxx-builder/.buildkite-agent/builds/aaa2661d6269-1/llvm-project/libcxx-ci/libcxx/test/libcxx/algorithms/specialized.algorithms/special.mem.concepts/nothrow_sentinel_for.compile.pass.cpp --target=x86_64-unknown-linux-gnu -nostdinc++ -I /home/libcxx-builder/.buildkite-agent/builds/aaa2661d6269-1/llvm-project/libcxx-ci/build/generic-modules/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/aaa2661d6269-1/llvm-project/libcxx-ci/build/generic-modules/include/c++/v1 -I /home/libcxx-builder/.buildkite-agent/builds/aaa2661d6269-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -fmodules -fcxx-modules -Werror -Wall -Wctad-maybe-unsupported -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -Werror=thread-safety -Wuser-defined-warnings -fsyntax-only

Event Timeline

asb created this revision.Jan 22 2023, 10:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2023, 10:09 PM
asb requested review of this revision.Jan 22 2023, 10:09 PM
asb updated this revision to Diff 491237.Jan 22 2023, 10:40 PM

Removed FIXME missed in initial version.

luismarques accepted this revision.Jan 23 2023, 4:13 AM

LGMT.

clang/lib/CodeGen/TargetInfo.cpp
591

Nit: the !AsIfNoUniqueAddr condition could be tested before the !FD->hasAttr<NoUniqueAddressAttr>() condition, and I imagine that would be slightly cheaper for the AsIfNoUniqueAddr = true case?

This revision is now accepted and ready to land.Jan 23 2023, 4:13 AM
asb updated this revision to Diff 491781.Jan 24 2023, 7:17 AM
asb marked an inline comment as done.
  • Address review comments
  • Refresh after adding zero-length array tests (and fix a bug this unearthed).
  • Add to release notes
luismarques accepted this revision.Jan 24 2023, 7:24 AM
asb added a comment.Jan 25 2023, 8:06 AM

Thanks Luis for the quick review. As an important part of this is trying to align with gcc/g++ I'd really appreciate a review from @kito-cheng too if you have the time (thanks in advance!).

asb added a comment.Jan 31 2023, 1:56 AM

Thanks Luis for the quick review. As an important part of this is trying to align with gcc/g++ I'd really appreciate a review from @kito-cheng too if you have the time (thanks in advance!).

Friendly ping to Kito as I spotted you mentioned you're back from vacation!

asb added a comment.Feb 7 2023, 3:28 AM

Friendly ping on this (I think mostly directed at @kito-cheng who was hoping to find time to review the linked abi issue).

kito-cheng accepted this revision.Feb 10 2023, 1:49 AM

LGTM, Verified with GCC and two clang w/ and w/o this patch, clang with this patch is matching GCC's behavior.

Also created an PR on psABI to clarify that: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/365

luismarques added inline comments.Tue, Mar 14, 9:41 AM
clang/test/CodeGen/RISCV/abi-empty-structs.c
21–22

Shouldn't you mention the array exception here? (which you now cover in test_s7 and test_s8).