rjmccall added a comment. In D111293#3049633 <https://reviews.llvm.org/D111293#3049633>, @jdoerfert wrote:
>> CodeGenFunction::InitTempAlloca() inits the static alloca within the entry >> block which may *not* necessarily be correct always. > > FWIW, for all uses this was correct. The point of the function was exactly to > do what you state here as "potentially incorrect". > The documentation explicitly states "and the initializer must be valid in the > entry block (i.e. it must either be a constant or an argument value)." > The rest of the reasoning that follows for this patch is consequently mood. > > While I don't expect this to have a real negative impact on performance it > certainly could if we are somewhat sensitive to the additional stores that > are now executed prior to an event which requires a temporary storage > location (in OpenMP). It's not potentially incorrect because of dominance problems, it's potentially incorrect because it means the initialization is performed exactly once per call to the enclosing function rather than once per evaluation of the code in question, which generally means it may have been overwritten as part of a prior evaluation. For example, the ObjC code in this patch is trying to zero-initialize a temporary to handle the case where the receiver is nil, but I believe there are situations where that temporary may be modified following the call, and so it's really necessary to zero-initialize prior to each message send, not just once in the prologue. (With that said, what this code is doing with a PHI is silly, and it should just be zeroing the same temporary used for the call return.) Some of the OpenMP uses seem to be assuming that they're emitting to the start of a function and would be clearer if they just emitted the stores normally. The rest seem to be passing the address of a variable holding `i32 0` to a call argument. If all of those calls promise not to modify this argument, we could indeed hoist this store to the prologue. Better yet, we could just pass the address of a constant global variable and not do any stores at all. In either case, having a general `InitTempAlloca` doesn't seem like the best approach. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111293/new/ https://reviews.llvm.org/D111293 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
