This is an archive of the discontinued LLVM Phabricator instance.

[C2x] Implement support for empty brace initialization (WG14 N2900 and WG14 N3011)
ClosedPublic

Authored by aaron.ballman on Mar 31 2023, 12:11 PM.

Details

Summary

This implements support for allowing {} to consistently zero initialize objects. We already supported most of this work as a GNU extension, but the C2x feature goes beyond what the GNU extension allowed.

The changes in this patch are:

This functionality is exposed as an extension in all older C modes (same as the GNU extension was), but does *not* allow the extension for VLA initialization in C++ due to concern about handling non-trivially constructible types.

Diff Detail

Event Timeline

aaron.ballman created this revision.Mar 31 2023, 12:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2023, 12:11 PM
aaron.ballman requested review of this revision.Mar 31 2023, 12:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2023, 12:11 PM

Oops, previous diff missed a change that was only locally staged. Fixed.

Fixed some test failures found by precommit CI.

Another round of fixing what post commit CI found.

So not necessary to this patch, but I suspect the non-trivially constructible types could just be solved the same way we do for zero-init of these types in fixed-size arrays, right? Its a bit more complicated, but the IR is probably just as do-able: https://godbolt.org/z/Gqf65xr8M
There, in the in it of %6 is the only place the length matters, since it looks like we do a 'begin/end' type emit for it. %2 is kept as the 'current element being processed', and %6 is the 'end', and %5 is the 'begin'.

That said, only a few small comments/questions.

clang/lib/CodeGen/CGExprAgg.cpp
1670

I'd probably prefer an assert for 'initializer is empty' here to confirm this, but else this seems fine.

clang/test/C/C2x/n2900_n3011_2.c
42

This is strange... why is this not an error? What is this supposed to mean?

56

These MEM# names are horrible to read :/ But the test is doing the right thing it appears.

97

A VLA of these things still works right? Is that worth a test?

aaron.ballman marked 4 inline comments as done.Apr 3 2023, 10:34 AM

So not necessary to this patch, but I suspect the non-trivially constructible types could just be solved the same way we do for zero-init of these types in fixed-size arrays, right? Its a bit more complicated, but the IR is probably just as do-able: https://godbolt.org/z/Gqf65xr8M
There, in the in it of %6 is the only place the length matters, since it looks like we do a 'begin/end' type emit for it. %2 is kept as the 'current element being processed', and %6 is the 'end', and %5 is the 'begin'.

That said, only a few small comments/questions.

It's possible, but I'm also not convinced extending C++'s already inscrutable initialization rules is a good idea, especially given that it relates to VLAs (which are already a bit questionable as an extension to C++ IMO).

clang/lib/CodeGen/CGExprAgg.cpp
1670

I can add that assert, sure.

clang/test/C/C2x/n2900_n3011_2.c
42

Heh, this one took me a few minutes to convince myself is correct. This creates a zero-length array, which we support as an extension. There's a corresponding Sema test on n2900_n3011.c:18 that shows the behavior of the extension diagnostics in this case.

56

If you have suggestions for better names, I'm happy to use them.

97

I can add a test for that, sure.

erichkeane added inline comments.Apr 3 2023, 10:52 AM
clang/test/C/C2x/n2900_n3011_2.c
52

so 'entry' isn't really a stable name AFAIK. So this might fail in test configs that do the 'erase names' thing. That said, a buildbot hasn't caught one of those in a long time, so *shrug*.

56

I: I_PTR
MEM0: I_VAL
MEM1: NUM_ELTS (or I_AS_64B?).
MEM3: COPY_BYTES

Though, this all becomes a bit easier with 'i' in code being named num_elts or something.
NUM_ELTS_PTR
NUM_ELTS
NUM_ELTS_EXT
BYTES_TO_COPY

aaron.ballman marked 4 inline comments as done.

Update based on review feedback:

  • Added an assertion to codegen that a VLA with an initializer is an empty initializer
  • Updated the codegen tests to use better identifiers
  • Added a codegen test for empty initialization of VLA of structures
erichkeane accepted this revision.Apr 3 2023, 11:36 AM
This revision is now accepted and ready to land.Apr 3 2023, 11:36 AM
This revision was landed with ongoing or failed builds.Apr 3 2023, 12:23 PM
This revision was automatically updated to reflect the committed changes.