[CFE][Codegen][In-progress] Remove CodeGenFunction::InitTempAlloca()

[hsmhsm]

Summary

CodeGenFunction::InitTempAlloca() inits the static alloca within the
entry block which may *not* necessarily be correct always.

For example, the current instruction insertion point (pointed by the
instruction builder) could be a program point which is hit multiple
times during the program execution, and it is expected that the static
alloca is initialized every time the program point is hit.

Hence remove CodeGenFunction::InitTempAlloca(), and initialize the
static alloca where the instruction insertion point is at the moment.

This patch, as a starting attempt, removes the calls to
CodeGenFunction::InitTempAlloca() which do not have any side effect on
the lit tests.

[rjmccall]

We should take the opportunity to add a test here for the ObjC case. It should be testable with something like this, where you should see the store inside of the loop instead of once in the prologue:

// RUN: %clang_cc1 -triple x86_64-unknown-freebsd -S -emit-llvm -fobjc-runtime=gnustep-2.0 -o - %s | FileCheck %s

typedef struct {
  int x[12];
} Big;

@protocol P
- (Big) foo;
@end

void test(id<P> p) {
  for (;;) {
    Big big = [p foo];
  }
}

[jdoerfert]

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).

[rjmccall]

In 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.

[hsmhsm]

Add lit test - CodeGenObjC/static-alloc-init.m

[rjmccall]

Hmm. I decided the ObjC case needed a more complex change and went ahead and committed that fix here: https://github.com/llvm/llvm-project/commit/5ab6ee75994d. That obviates the need for the new test; sorry for the runaround. Please rebase and I'll review.

[hsmhsm]

Rebase to latest trunk.

[rjmccall]

Patch LGTM, but please remove the test, it's been folded into a test I added in my patch.

[hsmhsm]

Remove newly added test CodeGenObjC/static-alloc-init.m

clang/lib/CodeGen/CodeGenFunction.cpp

#undef SANITIZER
   ReturnBlock = getJumpDestInCurrentScope("return");
 
   Builder.SetInsertPoint(EntryBB);
 
   // If we're checking the return value, allocate space for a pointer to a
   // precise source location of the checked return statement.
   if (requiresReturnValueCheck()) {
     ReturnLocation = CreateDefaultAlignTempAlloca(Int8PtrTy, "return.sloc.ptr");
-    InitTempAlloca(ReturnLocation, llvm::ConstantPointerNull::get(Int8PtrTy));
+    Builder.CreateStore(llvm::ConstantPointerNull::get(Int8PtrTy),
+                        ReturnLocation);
   }
 
   // Emit subprogram debug descriptor.
   if (CGDebugInfo *DI = getDebugInfo()) {
     // Reconstruct the type from the argument list so that implicit parameters,
     // such as 'this' and 'vtt', show up in the debug info. Preserve the calling
     // convention.
     DI->emitFunctionStart(GD, Loc, StartLoc,