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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
