hsmhsm marked 2 inline comments as done. hsmhsm added inline comments.
================ Comment at: clang/lib/CodeGen/CGExpr.cpp:115 + if (AllocaInsertedAtAllocaInsertPt) + AllocaAddrSpaceInsertPt = dyn_cast<llvm::Instruction>(V)->getIterator(); } ---------------- arichardson wrote: > Shouldn't this use `cast` instead? You are right. But, in the updated patch, this code does not exist anymore. ================ Comment at: clang/lib/CodeGen/CodeGenFunction.h:392 + /// order immediately after all static allocas. + llvm::BasicBlock::iterator AllocaAddrSpaceInsertPt; + ---------------- rjmccall wrote: > Please call this something like `PostAllocaInsertPt`. Instead of eagerly > creating it, please create it lazily: add an accessor like > `getPostAllocaInsertPoint()` which creates (and saves here) an instruction > that immediately follows `AllocaInsertPt` if this is currently null. I think > you should make your own instruction so that we don't mess up any code that > might rely on temporarily creating and then removing dead instructions. > > Please use an `llvm::AssertingVH<llvm::Instruction>`. I think that can > handle holding a null value. > > The comment here should be more general, like "a place in the prologue where > code can be inserted that will be dominated by all the static allocas." You > don't need to talk about `addrspacecast`s specifically; that's just one > possible use case for this. > > Also, it's either "`addrspacecast`" (using the IR name of the operation) or > "address space cast" (writing out the operation in normal English words); > don't run together `addressspace` as if it were a keyword when it isn't. Thanks. Fixed the above review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110257/new/ https://reviews.llvm.org/D110257 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
