jfb added inline comments.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1647
+ QualType Ty) {
+ auto *I8Ptr = CGF.Builder.CreateBitCast(Ptr, CGF.Int8PtrTy);
+ auto *Zero = ConstantInt::get(CGF.Int8Ty, 0);
----------------
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.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1652
+ auto Element = CGF.Builder.CreateGEP(I8Ptr, Index);
+ CGF.Builder.CreateAlignedStore(Zero, Element, MaybeAlign());
+ };
----------------
You should use `alignmentAtOffset` here.
================
Comment at: clang/test/CodeGenCXX/builtin-zero-non-value-bits-codegen.cpp:16
+ Bar f;
+};
+
----------------
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.
================
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]]
----------------
It would help read the tests if you had a comment on top of each store, for
example here "padding byte X".
================
Comment at: clang/test/CodeGenCXX/builtin-zero-non-value-bits-codegen.cpp:46
+void test(Baz *baz) {
+ __builtin_zero_non_value_bits(baz);
+}
----------------
It would be useful to see a test for arrays with a type that contains tail
padding.
================
Comment at: clang/test/CodeGenCXX/builtin-zero-non-value-bits.cpp:160
+
+int main() {
+ testAllForType<32, 16, char>(11, 22, 33, 44);
----------------
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 :)
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
