rjmccall added a comment.

This patch seems to be confused.  You're making two changes.  In one of them, 
you're trying to prevent `addrspacecast`s from being interleaved with the 
sequence of `alloca`s in the function prologue.  In the other, you're moving 
the store emitted by `InitTempAlloca` so that it becomes interleaved with the 
sequence of `alloca`s, but only when there's an `addrspacecast`.

Now, I should say that `InitTempAlloca` is generally a problematic function.  
Its purpose is to put an initialization in the prologue of the function so that 
it always happens prior to some other code executing.  This is rarely useful, 
though, because the memory is usually tied to some specific feature in the 
code, and as Johannes says, that place in the code may be reachable multiple 
times, and the memory typically needs to be freshly initialized each time.  
Using `InitTempAlloca` is therefore frequently wrong, and I'm honestly not sure 
there's any good reason to use it.  Looking at the calls, some of them know 
that they're in the prologue, and so it should be fine to simply emit a store 
normally.  Other calls seem quite suspect, like the one in CGObjCGNU.cpp.  And 
even if it's semantically okay, it's potentially doing unnecessary work 
up-front when it only really needs to happen if that path is taken.

So I don't really care about aesthetic differences in the code created by 
`InitTempAlloca`, because we should just remove it completely.

If we really care about not interleaving things with the `alloca` sequence — 
and right now I am not convinced that we do, because contra the comments and 
description of this patch, this is not an LLVM requirement of any sort — I 
think we should lazily create a second InsertPt instruction after the 
`AllocaInsertPt` and insert all the secondary instruction prior to that so that 
they appear in source order.


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

Reply via email to