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

[John McCall via Phabricator via cfe-commits]

rjmccall added a reviewer: ABataev.
rjmccall added a comment.

This mostly looks good, but I'd like Alexey Bataev to sign off, since he authored the OpenMP support.



================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:2124
                                          /*Name=*/".bound.zero.addr");
-    CGF.InitTempAlloca(ZeroAddrBound, CGF.Builder.getInt32(/*C*/ 0));
+    CGF.Builder.CreateStore(CGF.Builder.getInt32(/*C*/ 0), ZeroAddrBound);
     llvm::SmallVector<llvm::Value *, 16> OutlinedFnArgs;
----------------
This seems innocuous since `ZeroAddrBound` does not otherwise change in the function, assuming the runtime call doesn't modify it.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:4784
+      CGF.Builder.CreateStore(llvm::ConstantInt::get(CGF.IntPtrTy, 0),
+                              NumLVal.getAddress(CGF));
       llvm::Value *PrevVal = CGF.EmitLoadOfScalar(NumLVal, E->getExprLoc());
----------------
The original generated code here is very strange:

```
%depobj.size.addr = alloca i64
...  
store i64 0, i64* %depobj.size.addr    // currently in prologue
...
%numdeps = load // from the size field of the struct kmp_depend_info which precedes all dependent objects
...
%tmp = load i64, i64* %depobj.size.addr
%sum = add nuw i64 %tmp, i64 %numdeps
store i64 %sum, %i64* %depobj.size.addr
...
// and then in the one place we actually call this function, we
// immediately sum up all the %sums for all the dependent objects
```

The funny thing about this code is that, because the zeroing of `%depobj.size.addr` is only done in the prologue, it increases by the current element count of the depend object every time if this code is run multiple times, which I think cannot possibly be correct.  Notably, the code to actually copy dependency data does not seem to have this bug because `PosLVal` is zeroed immediately.

So I'm pretty sure this change is actually a bug fix in the treatment of `depend(depobj : ...)`.  However, I'd really like Alexey to review and approve, because if `%depobj.size.addr` is *not* supposed to accumulate values in a loop, then I really don't understand why it exists at all.




================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:1503
                                                       /*Name=*/".zero.addr");
-  CGF.InitTempAlloca(ZeroAddr, CGF.Builder.getInt32(/*C*/ 0));
+  CGF.Builder.CreateStore(CGF.Builder.getInt32(/*C*/ 0), ZeroAddr);
   llvm::SmallVector<llvm::Value *, 16> OutlinedFnArgs;
----------------
This seems innocuous since `ZeroAddr` does not otherwise change in the function, assuming the runtime call doesn't modify it.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:3491
                                                       /*Name=*/".zero.addr");
-  CGF.InitTempAlloca(ZeroAddr, CGF.Builder.getInt32(/*C*/ 0));
+  CGF.Builder.CreateStore(CGF.Builder.getInt32(/*C*/ 0), ZeroAddr);
   // Get the array of arguments.
----------------
This seems innocuous since `ZeroAddr` does not otherwise change in the function, assuming the runtime call doesn't modify it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111316/new/

https://reviews.llvm.org/D111316