This is an archive of the discontinued LLVM Phabricator instance.

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

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

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.Mar 14 2023, 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).

This revision was landed with ongoing or failed builds.Jul 24 2023, 2:25 AM
This revision was automatically updated to reflect the committed changes.
rogfer01 added inline comments.Jul 24 2023, 8:53 AM
clang/lib/CodeGen/Targets/RISCV.cpp
178 ↗(On Diff #543432)

I've observed (based on manually added tracing) that we may return true here with Field1Ty == nullptr and Field2Ty == nullptr.

Then RISCVABIInfo::coerceAndExpandFPCCEligibleStruct receives these two types and appends Field1Ty it into UnpaddedCoerceElts and then if Field2Ty == nullptr it calls ABIArgInfo::getCoerceAndExpand (around line 280 of clang/lib/CodeGen/Targets/RISCV.cpp) which then tries to do a dyn_cast on a null value and then an assertion fires (around line 256 of clang/include/clang/CodeGen/CGFunctionInfo.h)

I can reproduce this with the llvm-testsuite. Apologies that I have not managed to create a small reproducer yet.

asb reopened this revision.Jul 24 2023, 9:01 AM

Reopening as I've reverted following Roger's bug report.

clang/lib/CodeGen/Targets/RISCV.cpp
178 ↗(On Diff #543432)

Thank you, I've reverted until this can be investigated and fixed.

This revision is now accepted and ready to land.Jul 24 2023, 9:01 AM
SixWeining added inline comments.
clang/lib/CodeGen/Targets/RISCV.cpp
178 ↗(On Diff #543432)

LoongArch has the same issue and I write a reproducer:

$ cat test.cpp
#include <algorithm>
#include <vector>

static bool check(int i) {
  return i == 2;
}

int main()
{
  std::vector<int> v{1, 2, 3, 2, 2};
  //return std::count_if(v.begin(), v.end(), check); // ok
  return std::count_if(v.begin(), v.end(), [](int i) { return i == 2; }); // error
}

Let's investigate how to fix it.

evandro removed a subscriber: evandro.Jul 25 2023, 9:43 AM
asb updated this revision to Diff 544349.Jul 26 2023, 6:57 AM

I've updated the patch to fix the reported issue (adding an additional check to detectFPCCEligibleStruct). What I haven't been able to do so far is to extract a test case that shows the issue indicated by @SixWeining without relying on libcxx headers. I may well be missing something obvious here.

This comment was removed by rogfer01.
rogfer01 added a comment.EditedJul 27 2023, 12:58 AM

I didn't have libcxx handy but @SixWeining testcase also fails with libstdcxx so I manually expanded what libstdcxx does and removed the templates.

With the new change this does not fail anymore.

typedef decltype((int *)2 - (int *)1) my_ptrdiff_t;

struct my_lambda {
  bool operator()(int i) { return i == 2; }
};

struct my_iter_pred {
  my_lambda _M_pred;

  explicit my_iter_pred(my_lambda __pred)
      : _M_pred(/* move */ static_cast<my_lambda &&>(__pred)) {}

  bool operator()(int *__it) { return bool(_M_pred(*__it)); }
};

my_ptrdiff_t __my_count_if(int *__first, int *__last, my_iter_pred __pred) {
  my_ptrdiff_t __n = 0;
  for (; __first != __last; ++__first)
    if (__pred(__first))
      ++__n;
  return __n;
}

inline my_iter_pred my_pred_iter(my_lambda __my_lambda) {
  return my_iter_pred(/* move */ static_cast<my_lambda &&>(__my_lambda));
}

inline my_ptrdiff_t my_count_if(int *__first, int *__last,
                                my_lambda __my_lambda) {
  return __my_count_if(__first, __last, my_pred_iter(__my_lambda));
}

int main() {
  int v[] = {1, 2, 3, 2, 2};
  return my_count_if(v, v + sizeof(v) / sizeof(*v), my_lambda());
}

I tried to remove the my_pred_iter / my_iter_pred wrapper but then the previous change would not fail anymore.

Hope this helps.

asb updated this revision to Diff 546012.Aug 1 2023, 5:47 AM

This update includes test coverage for the bug that was fixed in the reverted version of the patch.

Thanks @rogfer01 for the smaller test case. To my surprise, cvise stripped out almost everything and test_s9 in abi-empty-structs.c reliably caused the assert for me.

Re-review appreciated in order to get this landed and ideally cherry-picked for the 17.x.

asb requested review of this revision.Aug 1 2023, 5:47 AM
rogfer01 accepted this revision.Aug 7 2023, 1:33 AM

Thanks for the update @asb. LGTM.

This revision is now accepted and ready to land.Aug 7 2023, 1:33 AM