[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

[JF Bastien via Phabricator via cfe-commits]

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