Page MenuHomePhabricator

[Builtin] Add __builtin_zero_non_value_bits.
Needs ReviewPublic

Authored by zoecarver on Sep 19 2020, 1:25 PM.

Details

Summary

Adds __builtin_zero_non_value_bits to zero all padding bits of a struct. This builtin should match the behavior of those in NVCC and GCC (and MSVC?). There are some tests in this patch but hopefully we'll also get tests from other compilers (so all builtins can be as similar as possible).

I'm planning to add support for unions, bitfields (both as members and members of sub-objects), and booleans as follow up patches.

Diff Detail

Event Timeline

zoecarver created this revision.Sep 19 2020, 1:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 19 2020, 1:25 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
zoecarver requested review of this revision.Sep 19 2020, 1:25 PM
zoecarver edited the summary of this revision. (Show Details)Sep 19 2020, 1:32 PM
zoecarver added reviewers: jfb, rsmith, Bigcheese, __simt__.
jfb added inline comments.Sep 19 2020, 4:14 PM
clang/lib/CodeGen/CGBuiltin.cpp
1682

Typo in "fields".

clang/test/CodeGenCXX/builtin-zero-non-value-bits.cpp
161

Usually CodeGen tests will use lit to check the emitted IR matches expectations. I think that's what you want to do here.

Remember to test volatile qualified pointers, as well as address spaces too.

clang/test/SemaCXX/builtin-zero-non-value-bits.cpp
12

You should also check incomplete types, vector, variable width integers, const qualified.

zoecarver added inline comments.Sep 20 2020, 1:35 PM
clang/test/CodeGenCXX/builtin-zero-non-value-bits.cpp
161

What's a good place for me to put this end-to-end test?

zoecarver retitled this revision from Summary: [Builtin] Add __builtin_zero_non_value_bits. to [Builtin] Add __builtin_zero_non_value_bits..Sep 22 2020, 9:32 AM
zoecarver updated this revision to Diff 293606.Sep 22 2020, 5:46 PM
zoecarver edited the summary of this revision. (Show Details)
  • Add more test cases.
  • Fix typo.
  • Add codegen tests.
zoecarver marked 2 inline comments as done and an inline comment as not done.Sep 22 2020, 5:48 PM
jfb added inline comments.Sep 23 2020, 10:10 AM
clang/lib/CodeGen/CGBuiltin.cpp
1647

I'd like to hear @rsmith's thoughts on this approach, in particular w.r.t. aliasing concerns. I also wonder if below the GEPs should be inbounds, depending on how they're created.

1652

You should use alignmentAtOffset here.

clang/test/CodeGenCXX/builtin-zero-non-value-bits-codegen.cpp
17

It would be helpful to have a comment with the final layout of the struct, including padding. Give each padding field a name, and reference them in the IR check below.

27

It would help read the tests if you had a comment on top of each store, for example here "padding byte X".

47

It would be useful to see a test for arrays with a type that contains tail padding.

clang/test/CodeGenCXX/builtin-zero-non-value-bits.cpp
161

I'm not sure, I don't usually add this type of test :)

zoecarver marked 3 inline comments as done.Sep 27 2020, 10:29 PM
zoecarver added inline comments.
clang/test/CodeGenCXX/builtin-zero-non-value-bits-codegen.cpp
17

Done. I've named each padding field as "PAD_X" so below it should be clear what fields are being stored without a comment.

27

See above comment.

47

Hmm, this test case doesn't seem to be working. I'll investigate further.

clang/test/CodeGenCXX/builtin-zero-non-value-bits.cpp
161

Even if it's a bit unconventional, I think it would be good to have this type of test. I think we should try to cover as many test cases as possible because it's important that this builtin both doesn't zero non-padding bits and does zero all padding bits. And it wouldn't be practical to add the 100+ test cases covered here as codegen tests.

zoecarver marked 3 inline comments as done.
  • Add UnsizedTail codegen test
zoecarver marked an inline comment as done.Sep 30 2020, 5:57 PM
zoecarver added inline comments.
clang/test/CodeGenCXX/builtin-zero-non-value-bits-codegen.cpp
47

OK, I've added that. Just to clarify, you mean a type that contains a constant array type of types with tail padding (i.e., Bar [2])?

zoecarver updated this revision to Diff 295445.Sep 30 2020, 5:58 PM
  • Support constant arrays
  • Format changes with clang-format
zoecarver added inline comments.Sep 30 2020, 6:00 PM
clang/lib/CodeGen/CGBuiltin.cpp
1652

I'm using CharUnits::One().alignmentAtOffset to here because this type will always have a size of 1 because it's an i8 ptr.

zoecarver added inline comments.Sep 30 2020, 6:08 PM
clang/lib/CodeGen/CGBuiltin.cpp
1704

Is it OK to possibly create hundreds of stores here? I assume later optimizations will catch this and turn it into a loop or a call to memset or something. But this could potentially be harmful to code size.

@jfb (and others), friendly ping. Are there any other changes (especially to the logic) that you'd like me to make?

clang/lib/CodeGen/CGBuiltin.cpp
1647

Hmm, I don't know much about it but, I think these could be inbound. Because we will never actually go beyond the size of the llvm type.

clang/test/CodeGenCXX/builtin-zero-non-value-bits-codegen.cpp
47

Let me know if there are more codegen tests you'd like me to add. Happy to add them. (Maybe one for vector types, or volatile, or one that doesn't have an non-value bits?)

jwakely added a subscriber: jwakely.EditedFri, Nov 20, 7:02 AM

As of a few hours ago, GCC has __builtin_clear_padding, see https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005fclear_005fpadding for the docs.

As of a few hours ago, GCC has __builtin_clear_padding, see https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005fclear_005fpadding for the docs.

Great. Then I'll update this to be named __builtin_clear_padding.

jfb added a comment.Fri, Nov 20, 1:49 PM

Let's make sure that we follow the same semantics that GCC does, particularly w.r.t. union, bitfields, and padding at the end of an object (whether it's in an array or not).

Let's make sure that we follow the same semantics that GCC does, particularly w.r.t. union, bitfields, and padding at the end of an object (whether it's in an array or not).

Agreed. I'm planning to run some tests tomorrow once the nightly build has updated.

I've resumed looking at the library code.

How should I check for support? Is it going to be e.g. __has_feature(__builtin_clear_padding)?

How should I check for support? Is it going to be e.g. has_feature(builtin_clear_padding)?

I'm not sure __has_feature will work but __has_builtin should work on both Clang and GCC.

@jwakely It looks like UnsizedTail causes a crash.

Other than that all the tests in this PR pass. It also looks like there's (at least) some support for unions and bitfields. This patch doesn't support those but I'm planning to add support as a follow-up.

@jwakely It looks like UnsizedTail causes a crash.

Filed https://gcc.gnu.org/PR97943 for that. Avoiding the crash is trivial, deciding what we want to do exactly for flexible array members (which are beyond the C++ standard) is harder.

Filed https://gcc.gnu.org/PR97943 for that. Avoiding the crash is trivial, deciding what we want to do exactly for flexible array members (which are beyond the C++ standard) is harder.

Yes, and we should try to do the same thing in this case. Currently, this implementation clears any padding bits that might come before the flexible array member but doesn't attempt to clear any of the array's padding bits (which I'm pretty sure wouldn't be feasible). So, what we're really deciding is whether or not to error for these types.