tbaeder added inline comments.
================ Comment at: clang/lib/AST/Interp/InterpBlock.h:97 void invokeCtor() { - std::memset(data(), 0, getSize()); + std::memset(rawData(), 0, Desc->getAllocSize()); if (Desc->CtorFn) ---------------- aaron.ballman wrote: > tbaeder wrote: > > aaron.ballman wrote: > > > tbaeder wrote: > > > > aaron.ballman wrote: > > > > > tbaeder wrote: > > > > > > aaron.ballman wrote: > > > > > > > Why do we want to overwrite the metadata here? > > > > > > This is only called after creating an new block, so nothing is > > > > > > being overwritten, the metadata hasn't been filled-in yet. > > > > > Sounds like a good reason not to memset over that block then; it's > > > > > useless work that will be thrown away anyway (I worry we may come to > > > > > rely on this zero init accidentally)? > > > > FWIW I looked into this and I think zeroing everything is what we want, > > > > so the initmap is null initially > > > Hmmm, would it make more sense to have the init map setting that itself > > > (in `allocate()`) rather than relying on `invokeCtor()` to do it? > > It's a bit blurry to me regarding the layering with Pointer/Descriptor. > > Descriptors don't know anything about the metadata, but pointers know that > > primitive arrays have an initmap. So to keep that consistent, we'd have to > > create a pointer to create the initmap. > > > > But I don't think it's unreasonable to expect zero-initialization for types > > used in the interpreter. > > It's a bit blurry to me regarding the layering with Pointer/Descriptor. > > Descriptors don't know anything about the metadata, but pointers know that > > primitive arrays have an initmap. So to keep that consistent, we'd have to > > create a pointer to create the initmap. > > > > But I don't think it's unreasonable to expect zero-initialization for types > > used in the interpreter. > > My concern is the overhead of doing initialization twice in the interpreter. > If we're going to zero init and then mostly write over the top of what we > initialized, the zero init busywork we don't need to perform. Secondarily, I > worry about relying on that zero init when it's not done as part of > initialization due to fragility (it's a multi-step init at this point; we > allocate, something else zeros, and something else fills in the data). Right, I understand the concerns. I have looked into using `data()` instead here, but that causes other problems with arrays of unknown size. (We assert in `getSize()`). But I know where the right place to initialize the initmap would be now. As for performance, this shouldn't make a difference to before, since the metadata is always small. We might of course want to get rid of the memset altogether, but both that //and// not zero-ing the metadata needs a different patch in any case. So I think this is okay for this patch. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135750/new/ https://reviews.llvm.org/D135750 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits