This is an archive of the discontinued LLVM Phabricator instance.

[clang][RISCV][test] Add test cases for empty structs and the FP calling conventions
ClosedPublic

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

Details

Summary

As reported in https://github.com/llvm/llvm-project/issues/58929, Clang currently differs from GCC in the handling of empty structs. This commit adds some test coverage for the handling of such structs.

Posting for review rather than committing directly because more eyes on our test coverage for in these areas would be useful.

A follow-up patch implements a fix to match g++.

Diff Detail

Event Timeline

asb created this revision.Jan 22 2023, 10:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2023, 10:06 PM
asb requested review of this revision.Jan 22 2023, 10:06 PM
luismarques accepted this revision.Jan 23 2023, 7:33 AM

LGTM but others should also chime in.

clang/test/CodeGen/RISCV/abi-empty-structs.c
102

Should we also test a case with an array size 0 like ct s { struct empty e[0]; float f; };? I think that's a GNU extension and I don't expect the result to be different but...

This revision is now accepted and ready to land.Jan 23 2023, 7:33 AM
asb added inline comments.Jan 23 2023, 12:01 PM
clang/test/CodeGen/RISCV/abi-empty-structs.c
102

Good idea and thanks for the review. As it stands, this patch has the bare minimum set of tests I'd be happy to land the ABI test fix. I'll try and do another pass to add in more tomorrow.

asb updated this revision to Diff 491780.Jan 24 2023, 7:16 AM
asb marked an inline comment as done.

Add zero-length array test cases.

luismarques added inline comments.Jan 24 2023, 7:29 AM
clang/test/CodeGen/RISCV/abi-empty-structs.c
1

Assume you updated this test with the not yet committed --full-function-signature option and forgot to remove that before updating the patch?
Please also check if the newly added %s below are supposed to be there.

asb added inline comments.Jan 24 2023, 7:38 AM
clang/test/CodeGen/RISCV/abi-empty-structs.c
1

The newly added %s are correct - it was an issue in the earlier patch that didn't impact update_cc_test_checks but did impact lit.

I think in order to land this ahead of the update_test_checks I'd remove this line and add perhaps add a FIXME comment to refresh the file once that patch lands.

luismarques accepted this revision.Jan 24 2023, 7:54 AM
This revision was landed with ongoing or failed builds.Mar 26 2023, 8:14 AM
This revision was automatically updated to reflect the committed changes.