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.Wed, Sep 30, 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.Wed, Sep 30, 5:58 PM
  • Support constant arrays
  • Format changes with clang-format
zoecarver added inline comments.Wed, Sep 30, 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.Wed, Sep 30, 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?)