[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

Mahesha S via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Oct 14 05:55:44 PDT 2021


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



More information about the cfe-commits mailing list