[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

Zoe Carver via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sun Sep 27 22:29:55 PDT 2020


zoecarver marked 3 inline comments as done.
zoecarver added inline comments.


================
Comment at: clang/test/CodeGenCXX/builtin-zero-non-value-bits-codegen.cpp:16
+  Bar f;
+};
+
----------------
jfb wrote:
> 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.
Done. I've named each padding field as "PAD_X" so below it should be clear what fields are being stored without a comment. 


================
Comment at: clang/test/CodeGenCXX/builtin-zero-non-value-bits-codegen.cpp:26
+// CHECK: [[FOO_RAW_PTR:%.*]] = bitcast %struct.Foo* [[FOO_BASE]] to i8*
+// CHECK: [[PAD_1:%.*]] = getelementptr i8, i8* [[FOO_RAW_PTR]], i32 1
+// CHECK: store i8 0, i8* [[PAD_1]]
----------------
jfb wrote:
> It would help read the tests if you had a comment on top of each store, for example here "padding byte X".
See above comment. 


================
Comment at: clang/test/CodeGenCXX/builtin-zero-non-value-bits-codegen.cpp:46
+void test(Baz *baz) {
+  __builtin_zero_non_value_bits(baz);
+}
----------------
jfb wrote:
> It would be useful to see a test for arrays with a type that contains tail padding.
Hmm, this test case doesn't seem to be working. I'll investigate further. 


================
Comment at: clang/test/CodeGenCXX/builtin-zero-non-value-bits.cpp:160
+
+int main() {
+  testAllForType<32, 16, char>(11, 22, 33, 44);
----------------
jfb wrote:
> zoecarver wrote:
> > jfb wrote:
> > > 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.
> > What's a good place for me to put this end-to-end test?
> I'm not sure, I don't usually add this type of test :)
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. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87974/new/

https://reviews.llvm.org/D87974



More information about the cfe-commits mailing list