jfb added a comment.
I've addressed @pcc's comments. I'll now update the patch to mandate a
`-Xclang` option for zero-init (otherwise it'll warn and pattern-init instead),
and remove documentation for zero-init. I'm also thinking of changing float
init to negative NaN with infinite scream payload.
================
Comment at: include/clang/Driver/ToolChain.h:355
+ virtual LangOptions::TrivialAutoVarInitKind
+ GetDefaultTrivialAutoVarInit() const {
+ return LangOptions::TrivialAutoVarInitKind::Uninitialized;
----------------
pcc wrote:
> I wouldn't introduce this function because nothing is overriding it (for now,
> at least).
I expect to do so for particular targets, so it's only a question of time, and
for now it makes internal testing easier (I can build my compiler and override
it for testing, before sending that upstream). I'd rather do it now, since it
reduces merge conflicts as we kick the tires internally.
================
Comment at: lib/CodeGen/CGDecl.cpp:977
+ constexpr uint32_t SmallValue = 0x000000AA;
+ if (Ty->isIntOrIntVectorTy()) {
+ unsigned BitWidth = llvm::cast<llvm::IntegerType>(
----------------
pcc wrote:
> Do you have test coverage for vectors?
`test/CodeGenCXX/auto-var-init.cpp` has those tests. I was lazy with this patch
and hadn't updated the tests yet, in case people asked me to change the values.
I've done so in this update.
================
Comment at: lib/CodeGen/CGDecl.cpp:1673
+ llvm::BasicBlock *ContBB = createBasicBlock("vla-init.cont");
+ EmitBlock(LoopBB);
+ llvm::PHINode *Cur =
----------------
pcc wrote:
> I think we need to check that the size is non-zero here. Otherwise we're
> going to clobber whatever comes after the VLA if its size is zero (I know
> that C forbids zero-sized VLAs, but I believe we support them as a GNU
> extension).
Indeed! Thanks for catching that. I thought I'd done it, and it turns out on O0
it wasn't doing anything bad, and neither was it in O2 because the GEPs were
marked inbounds so the codegen magically turned into a memset based on size!
================
Comment at: test/CodeGenCXX/trivial-auto-var-init.cpp:5
+
+// PATTERN-NOT: undef
+// ZERO-NOT: undef
----------------
pcc wrote:
> Since this is your only test involving structs and arrays, you probably want
> to test them beyond the fact that they don't contain undef. I also believe
> that this is testing for the absence of `undef` before the first label and
> not the total absence of `undef` in the output.
I was lazy with testing in case the design changed significantly. The updated
test explains what I was going for here :)
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54604/new/
https://reviews.llvm.org/D54604
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
