[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-25 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits 
fdeazeve added a comment.

https://reviews.llvm.org/D142160

Should address the incorrect handling of Mem2Reg, this is not the full picture 
though, as at the MIR level we handle these two differently:

  DEBUG_VALUE  !DIExpression ()  indirect// this what dbg.declare gets 
lowered to
  DEBUG_VALUE  !DIExpression (OP_deref)  // this is what dbg.value + 
deref gets lowered to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-17 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits 
fdeazeve added a comment.

The code in Mem2Reg (Local.cpp) is wrong in the presence of DIExprs, but by 
luck we conservatively don't do anything.

The function below is roughly doing: "if this store covers the entire size of 
the `DIVar`, rewrite the dbg declare into a dbg value.
This is only correct if the `DIExpr` is empty. In most cases, "luckily" the 
size of a pointer doesn't match the size of the `DIVar`, but it should be 
possible to come up with IR examples where we generate invalid debug 
information in the presence of a `OP_deref`.

  /// Inserts a llvm.dbg.value intrinsic before a store to an alloca'd value
  /// that has an associated llvm.dbg.declare or llvm.dbg.addr intrinsic.
  void llvm::ConvertDebugDeclareToDebugValue(DbgVariableIntrinsic *DII,
 StoreInst *SI, DIBuilder ) 
{
assert(DII->isAddressOfVariable() || isa(DII));
auto *DIVar = DII->getVariable();
assert(DIVar && "Missing variable");
auto *DIExpr = DII->getExpression();
DIExpr->startsWithDeref();
Value *DV = SI->getValueOperand();
  
DebugLoc NewLoc = getDebugValueLoc(DII);
  
if (!valueCoversEntireFragment(DV->getType(), DII)) {
  // FIXME: If storing to a part of the variable described by the 
dbg.declare,
  // then we want to insert a dbg.value for the corresponding fragment.
  LLVM_DEBUG(dbgs() << "Failed to convert dbg.declare to dbg.value: "
<< *DII << '\n');
  // For now, when there is a store to parts of the variable (but we do not
  // know which part) we insert an dbg.value intrinsic to indicate that we
  // know nothing about the variable's content.
  DV = UndefValue::get(DV->getType());
  Builder.insertDbgValueIntrinsic(DV, DIVar, DIExpr, NewLoc, SI);
  return;
}
  
Builder.insertDbgValueIntrinsic(DV, DIVar, DIExpr, NewLoc, SI);
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-17 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie added a comment.

In D141381#4056661 , @fdeazeve wrote:

> In hindsight, this should have been obvious.
> While SROA will not touch this:
>
>   define @foo(ptr %arg) {
>  call void @llvm.dbg.declare(%arg, [...], metadata !DIExpression())
>
> It completely destroys the debug information provided by:
>
>   define @foo(ptr %arg) {
>  %ptr_storage = alloca ptr
>  store ptr %arg, ptr %ptr_storage
>  call void @llvm.dbg.declare(%ptr_storage, [...], metadata 
> !DIExpression(DW_OP_deref))
>
> In other words, SROA rewrites the above to:
>
>   define @foo(ptr %arg) {
>  call void @llvm.dbg.declare(undef, [...], metadata 
> !DIExpression(DW_OP_deref))

Seems reasonable to me that SROA should be able to do a better/the right job 
here, for this and other places where the equivalent operation might occur... 
but this is hardly my wheelhouse/don't take that perspective as gospel.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-17 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits 
fdeazeve added a comment.

In D141381#4058327 , @jmorse wrote:

> Curious -- that feels like the kind of thing we should be able to support, 
> but adding more "special handling" to SROA isn't great.

Agreed! I'll have a look to see how far SROA is from being able to support 
this. Maybe it's almost there :)

> and if nothing optimises away the stack store

This is the part that concerns me a bit, since SROA removes the alloca+store 
because they have no uses other than the dbg.declare.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-17 Thread Jeremy Morse via Phabricator via cfe-commits 
jmorse added a comment.

Hi,

In D141381#4056661 , @fdeazeve wrote:

> While SROA will not touch this:
>
>   define @foo(ptr %arg) {
>  call void @llvm.dbg.declare(%arg, [...], metadata !DIExpression())
>
> It completely destroys the debug information provided by:
>
>   define @foo(ptr %arg) {
>  %ptr_storage = alloca ptr
>  store ptr %arg, ptr %ptr_storage
>  call void @llvm.dbg.declare(%ptr_storage, [...], metadata 
> !DIExpression(DW_OP_deref))

Curious -- that feels like the kind of thing we should be able to support, but 
adding more "special handling" to SROA isn't great.

One alternative line of investigation could be to keep the codegen changes 
you're adding, but leave the dbg.declare unmodified, i.e. referring to the 
argument value. The dbg.declare will become an indirect DBG_VALUE after isel, 
and if nothing optimises away the stack store, then LiveDebugValues (both 
flavours) might be able to track the store to stack and leave the variable 
homed there. I tried fiddling with 
llvm/test/DebugInfo/X86/spill-indirect-nrvo.ll to make that happen, however 
LiveDebugValues didn't follow the store, probably because it's to an alloca not 
a spill slot, thus there might be aliasing stores.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-16 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits 
fdeazeve added a comment.

In hindsight, this should have been obvious.
While SROA will not touch this:

  define @foo(ptr %arg) {
 call void @llvm.dbg.declare(%arg, [...], metadata !DIExpression())

It completely destroys the debug information provided by:

  define @foo(ptr %arg) {
 %ptr_storage = alloca ptr
 store ptr %arg, ptr %ptr_storage
 call void @llvm.dbg.declare(%ptr_storage, [...], metadata 
!DIExpression(DW_OP_deref))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-16 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits 
fdeazeve added a comment.

There is a real regression in 
`lldb/test/API/functionalities/param_entry_vals/basic_entry_values/` when this 
is compiled with O2 :

  __attribute__((noinline)) void func15(StructPassedViaPointerToTemporaryCopy 
S) 

It seems that, prior to this patch, `S` had debug information in O2 
 builds. I'll investigate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-16 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits 
fdeazeve added a comment.

Windows failures happening in Flang's MLIR stage, likely unrelated to this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-16 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG7e4447a17db4: [codegen] Store address of indirect arguments 
on the stack (authored by fdeazeve).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/CodeGen/CGDecl.cpp
  clang/test/CodeGen/aarch64-ls64.c
  clang/test/CodeGen/atomic-arm64.c
  clang/test/CodeGenCXX/amdgcn-func-arg.cpp
  clang/test/CodeGenCXX/debug-info.cpp
  clang/test/CodeGenCXX/derived-to-base-conv.cpp
  clang/test/CodeGenCoroutines/coro-params-exp-namespace.cpp
  clang/test/CodeGenCoroutines/coro-params.cpp
  clang/test/OpenMP/for_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_firstprivate_codegen.cpp
  clang/test/OpenMP/sections_firstprivate_codegen.cpp
  clang/test/OpenMP/single_firstprivate_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_firstprivate_codegen.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_firstprivate_codegen.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/teams_distribute_firstprivate_codegen.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_firstprivate_codegen.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/teams_distribute_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/teams_firstprivate_codegen.cpp

Index: clang/test/OpenMP/teams_firstprivate_codegen.cpp
===
--- clang/test/OpenMP/teams_firstprivate_codegen.cpp
+++ clang/test/OpenMP/teams_firstprivate_codegen.cpp
@@ -557,8 +557,10 @@
 // CHECK9-NEXT:  entry:
 // CHECK9-NEXT:[[THIS_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:[[S_ADDR:%.*]] = alloca ptr, align 8
+// CHECK9-NEXT:[[T_INDIRECT_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:store ptr [[THIS]], ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:store ptr [[S]], ptr [[S_ADDR]], align 8
+// CHECK9-NEXT:store ptr [[T]], ptr [[T_INDIRECT_ADDR]], align 8
 // CHECK9-NEXT:[[THIS1:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:[[TMP0:%.*]] = load ptr, ptr [[S_ADDR]], align 8
 // CHECK9-NEXT:call void @_ZN1SIfEC2ERKS0_2St(ptr nonnull align 4 dereferenceable(4) [[THIS1]], ptr nonnull align 4 dereferenceable(4) [[TMP0]], ptr [[T]])
@@ -784,8 +786,10 @@
 // CHECK9-NEXT:  entry:
 // CHECK9-NEXT:[[THIS_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:[[S_ADDR:%.*]] = alloca ptr, align 8
+// CHECK9-NEXT:[[T_INDIRECT_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:store ptr [[THIS]], ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:store ptr [[S]], ptr [[S_ADDR]], align 8
+// CHECK9-NEXT:store ptr [[T]], ptr [[T_INDIRECT_ADDR]], align 8
 // CHECK9-NEXT:[[THIS1:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:[[F:%.*]] = getelementptr inbounds [[STRUCT_S:%.*]], ptr [[THIS1]], i32 0, i32 0
 // CHECK9-NEXT:[[TMP0:%.*]] = load ptr, ptr [[S_ADDR]], align 8
@@ -932,8 +936,10 @@
 // CHECK9-NEXT:  entry:
 // CHECK9-NEXT:[[THIS_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:[[S_ADDR:%.*]] = alloca ptr, align 8
+// CHECK9-NEXT:[[T_INDIRECT_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:store ptr [[THIS]], ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:store ptr [[S]], ptr [[S_ADDR]], align 8
+// CHECK9-NEXT:store ptr [[T]], ptr [[T_INDIRECT_ADDR]], align 8
 // CHECK9-NEXT:[[THIS1:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:[[TMP0:%.*]] = load ptr, ptr [[S_ADDR]], align 8
 // CHECK9-NEXT:call void @_ZN1SIiEC2ERKS0_2St(ptr nonnull align 4 dereferenceable(4) [[THIS1]], ptr nonnull align 4 dereferenceable(4) [[TMP0]], ptr [[T]])
@@ -1012,8 +1018,10 @@
 // CHECK9-NEXT:  entry:
 // CHECK9-NEXT:[[THIS_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:[[S_ADDR:%.*]] = alloca ptr, align 8
+// CHECK9-NEXT:[[T_INDIRECT_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:store ptr [[THIS]], ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:store ptr [[S]], ptr [[S_ADDR]], align 8
+// CHECK9-NEXT:store ptr [[T]], ptr [[T_INDIRECT_ADDR]], align 8
 // CHECK9-NEXT:[[THIS1:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:[[F:%.*]] = getelementptr inbounds [[STRUCT_S_0:%.*]], ptr [[THIS1]], i32 0, i32 0
 // CHECK9-NEXT:[[TMP0:%.*]] = load ptr, ptr [[S_ADDR]], align 8
@@ -1318,8 +1326,10 @@
 // CHECK11-NEXT:  entry:
 // CHECK11-NEXT:[[THIS_ADDR:%.*]] = alloca ptr, align 4
 // CHECK11-NEXT:[[S_ADDR:%.*]] = alloca ptr, align 4
+// CHECK11-NEXT:[[T_INDIRECT_ADDR:%.*]] = alloca ptr, align 4
 // CHECK11-NEXT:store ptr

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-16 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits 
fdeazeve updated this revision to Diff 489502.
fdeazeve added a comment.

Rebased


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/CodeGen/CGDecl.cpp
  clang/test/CodeGen/aarch64-ls64.c
  clang/test/CodeGen/atomic-arm64.c
  clang/test/CodeGenCXX/amdgcn-func-arg.cpp
  clang/test/CodeGenCXX/debug-info.cpp
  clang/test/CodeGenCXX/derived-to-base-conv.cpp
  clang/test/CodeGenCoroutines/coro-params-exp-namespace.cpp
  clang/test/CodeGenCoroutines/coro-params.cpp
  clang/test/OpenMP/for_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_firstprivate_codegen.cpp
  clang/test/OpenMP/sections_firstprivate_codegen.cpp
  clang/test/OpenMP/single_firstprivate_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_firstprivate_codegen.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_firstprivate_codegen.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/teams_distribute_firstprivate_codegen.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_firstprivate_codegen.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/teams_distribute_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/teams_firstprivate_codegen.cpp

Index: clang/test/OpenMP/teams_firstprivate_codegen.cpp
===
--- clang/test/OpenMP/teams_firstprivate_codegen.cpp
+++ clang/test/OpenMP/teams_firstprivate_codegen.cpp
@@ -557,8 +557,10 @@
 // CHECK9-NEXT:  entry:
 // CHECK9-NEXT:[[THIS_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:[[S_ADDR:%.*]] = alloca ptr, align 8
+// CHECK9-NEXT:[[T_INDIRECT_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:store ptr [[THIS]], ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:store ptr [[S]], ptr [[S_ADDR]], align 8
+// CHECK9-NEXT:store ptr [[T]], ptr [[T_INDIRECT_ADDR]], align 8
 // CHECK9-NEXT:[[THIS1:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:[[TMP0:%.*]] = load ptr, ptr [[S_ADDR]], align 8
 // CHECK9-NEXT:call void @_ZN1SIfEC2ERKS0_2St(ptr nonnull align 4 dereferenceable(4) [[THIS1]], ptr nonnull align 4 dereferenceable(4) [[TMP0]], ptr [[T]])
@@ -784,8 +786,10 @@
 // CHECK9-NEXT:  entry:
 // CHECK9-NEXT:[[THIS_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:[[S_ADDR:%.*]] = alloca ptr, align 8
+// CHECK9-NEXT:[[T_INDIRECT_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:store ptr [[THIS]], ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:store ptr [[S]], ptr [[S_ADDR]], align 8
+// CHECK9-NEXT:store ptr [[T]], ptr [[T_INDIRECT_ADDR]], align 8
 // CHECK9-NEXT:[[THIS1:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:[[F:%.*]] = getelementptr inbounds [[STRUCT_S:%.*]], ptr [[THIS1]], i32 0, i32 0
 // CHECK9-NEXT:[[TMP0:%.*]] = load ptr, ptr [[S_ADDR]], align 8
@@ -932,8 +936,10 @@
 // CHECK9-NEXT:  entry:
 // CHECK9-NEXT:[[THIS_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:[[S_ADDR:%.*]] = alloca ptr, align 8
+// CHECK9-NEXT:[[T_INDIRECT_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:store ptr [[THIS]], ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:store ptr [[S]], ptr [[S_ADDR]], align 8
+// CHECK9-NEXT:store ptr [[T]], ptr [[T_INDIRECT_ADDR]], align 8
 // CHECK9-NEXT:[[THIS1:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:[[TMP0:%.*]] = load ptr, ptr [[S_ADDR]], align 8
 // CHECK9-NEXT:call void @_ZN1SIiEC2ERKS0_2St(ptr nonnull align 4 dereferenceable(4) [[THIS1]], ptr nonnull align 4 dereferenceable(4) [[TMP0]], ptr [[T]])
@@ -1012,8 +1018,10 @@
 // CHECK9-NEXT:  entry:
 // CHECK9-NEXT:[[THIS_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:[[S_ADDR:%.*]] = alloca ptr, align 8
+// CHECK9-NEXT:[[T_INDIRECT_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:store ptr [[THIS]], ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:store ptr [[S]], ptr [[S_ADDR]], align 8
+// CHECK9-NEXT:store ptr [[T]], ptr [[T_INDIRECT_ADDR]], align 8
 // CHECK9-NEXT:[[THIS1:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:[[F:%.*]] = getelementptr inbounds [[STRUCT_S_0:%.*]], ptr [[THIS1]], i32 0, i32 0
 // CHECK9-NEXT:[[TMP0:%.*]] = load ptr, ptr [[S_ADDR]], align 8
@@ -1318,8 +1326,10 @@
 // CHECK11-NEXT:  entry:
 // CHECK11-NEXT:[[THIS_ADDR:%.*]] = alloca ptr, align 4
 // CHECK11-NEXT:[[S_ADDR:%.*]] = alloca ptr, align 4
+// CHECK11-NEXT:[[T_INDIRECT_ADDR:%.*]] = alloca ptr, align 4
 // CHECK11-NEXT:store ptr [[THIS]], ptr [[THIS_ADDR]], align 4
 // CHECK11-NEXT:store ptr [[S]], ptr [[S_ADDR]], align 4
+// CHECK11-NEXT:store ptr [[T]], ptr [[T_INDIRECT_ADDR]],

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-12 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Sounds good


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-12 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits 
fdeazeve updated this revision to Diff 488611.
fdeazeve added a comment.

Address review comments:

- Remove unncesarry `const` qualifier from function declaration.
- Add an item in Clang's release notes.

I surveyed Clang's release notes in all major releases from 12.0 and couldn't
find anything similar to this, so I used my best judgement on where/how to
write it. Please let me know if you have suggestions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/CodeGen/CGDecl.cpp
  clang/test/CodeGen/aarch64-ls64.c
  clang/test/CodeGen/atomic-arm64.c
  clang/test/CodeGenCXX/amdgcn-func-arg.cpp
  clang/test/CodeGenCXX/debug-info.cpp
  clang/test/CodeGenCXX/derived-to-base-conv.cpp
  clang/test/CodeGenCoroutines/coro-params-exp-namespace.cpp
  clang/test/CodeGenCoroutines/coro-params.cpp
  clang/test/OpenMP/for_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_firstprivate_codegen.cpp
  clang/test/OpenMP/sections_firstprivate_codegen.cpp
  clang/test/OpenMP/single_firstprivate_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_firstprivate_codegen.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_firstprivate_codegen.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/teams_distribute_firstprivate_codegen.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_firstprivate_codegen.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/teams_distribute_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/teams_firstprivate_codegen.cpp

Index: clang/test/OpenMP/teams_firstprivate_codegen.cpp
===
--- clang/test/OpenMP/teams_firstprivate_codegen.cpp
+++ clang/test/OpenMP/teams_firstprivate_codegen.cpp
@@ -557,8 +557,10 @@
 // CHECK9-NEXT:  entry:
 // CHECK9-NEXT:[[THIS_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:[[S_ADDR:%.*]] = alloca ptr, align 8
+// CHECK9-NEXT:[[T_INDIRECT_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:store ptr [[THIS]], ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:store ptr [[S]], ptr [[S_ADDR]], align 8
+// CHECK9-NEXT:store ptr [[T]], ptr [[T_INDIRECT_ADDR]], align 8
 // CHECK9-NEXT:[[THIS1:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:[[TMP0:%.*]] = load ptr, ptr [[S_ADDR]], align 8
 // CHECK9-NEXT:call void @_ZN1SIfEC2ERKS0_2St(ptr nonnull align 4 dereferenceable(4) [[THIS1]], ptr nonnull align 4 dereferenceable(4) [[TMP0]], ptr [[T]])
@@ -784,8 +786,10 @@
 // CHECK9-NEXT:  entry:
 // CHECK9-NEXT:[[THIS_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:[[S_ADDR:%.*]] = alloca ptr, align 8
+// CHECK9-NEXT:[[T_INDIRECT_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:store ptr [[THIS]], ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:store ptr [[S]], ptr [[S_ADDR]], align 8
+// CHECK9-NEXT:store ptr [[T]], ptr [[T_INDIRECT_ADDR]], align 8
 // CHECK9-NEXT:[[THIS1:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:[[F:%.*]] = getelementptr inbounds [[STRUCT_S:%.*]], ptr [[THIS1]], i32 0, i32 0
 // CHECK9-NEXT:[[TMP0:%.*]] = load ptr, ptr [[S_ADDR]], align 8
@@ -932,8 +936,10 @@
 // CHECK9-NEXT:  entry:
 // CHECK9-NEXT:[[THIS_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:[[S_ADDR:%.*]] = alloca ptr, align 8
+// CHECK9-NEXT:[[T_INDIRECT_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:store ptr [[THIS]], ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:store ptr [[S]], ptr [[S_ADDR]], align 8
+// CHECK9-NEXT:store ptr [[T]], ptr [[T_INDIRECT_ADDR]], align 8
 // CHECK9-NEXT:[[THIS1:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:[[TMP0:%.*]] = load ptr, ptr [[S_ADDR]], align 8
 // CHECK9-NEXT:call void @_ZN1SIiEC2ERKS0_2St(ptr nonnull align 4 dereferenceable(4) [[THIS1]], ptr nonnull align 4 dereferenceable(4) [[TMP0]], ptr [[T]])
@@ -1012,8 +1018,10 @@
 // CHECK9-NEXT:  entry:
 // CHECK9-NEXT:[[THIS_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:[[S_ADDR:%.*]] = alloca ptr, align 8
+// CHECK9-NEXT:[[T_INDIRECT_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:store ptr [[THIS]], ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:store ptr [[S]], ptr [[S_ADDR]], align 8
+// CHECK9-NEXT:store ptr [[T]], ptr [[T_INDIRECT_ADDR]], align 8
 // CHECK9-NEXT:[[THIS1:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:[[F:%.*]] = getelementptr inbounds [[STRUCT_S_0:%.*]], ptr [[THIS1]], i32 0, i32 0
 // CHECK9-NEXT:[[TMP0:%.*]] = load ptr, ptr [[S_ADDR]], align 8
@@ -1318,8 +1326,10 @@
 // CHECK11-NEXT:  entry:
 // CHECK11-NEXT:[[THIS_ADDR:%.*]] = alloca

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-11 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits 
fdeazeve added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.h:494
+   CGBuilderTy ,
+   const bool UsePointerValue = false);
 

dblaikie wrote:
> fdeazeve wrote:
> > FWIW I used a `const` bool here to match the style already present in this 
> > class
> Examples of the things you were trying to be consistent with? Because the 
> other by-value parameter here isn't const at the top level & const at the top 
> level on function declaration parameters has no meaning, so especially here 
> it should probably be omitted (& probably also in the definition, but it's a 
> somewhat separate issue/has different tradeoffs)
The function on line 475 uses this same style:


```
  llvm::DILocalVariable *
  EmitDeclareOfAutoVariable(const VarDecl *Decl, llvm::Value *AI,
CGBuilderTy ,
const bool UsePointerValue = false);
```

That said, I agree with you and personally would not have added const here, 
I'll remove it and then later we can maybe clean up other functions in this file


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-11 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie added a comment.

In D141381#4045215 , @rjmccall wrote:

> That's about what I would expect.  One or two extra instructions per argument 
> that are trivially removed in debug builds.  Very small overall impact 
> because there just aren't very many of these arguments.
>
> I don't really see a need to post about this on Discourse, but it might be 
> worth a release note.

Fair enough - if you're cool with it. Yeah, release note wouldn't hurt.




Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:4826
+  unsigned ArgNo, CGBuilderTy ,
+  const bool UsePointerValue) {
   assert(CGM.getCodeGenOpts().hasReducedDebugInfo());

We don't usually use top-level const on parameters/locals like this, for 
consistency (eg: with `ArgNo`) please remove it. Otherwise it can lead to 
confusion that maybe there was a `&` intended, etc.



Comment at: clang/lib/CodeGen/CGDebugInfo.h:494
+   CGBuilderTy ,
+   const bool UsePointerValue = false);
 

fdeazeve wrote:
> FWIW I used a `const` bool here to match the style already present in this 
> class
Examples of the things you were trying to be consistent with? Because the other 
by-value parameter here isn't const at the top level & const at the top level 
on function declaration parameters has no meaning, so especially here it should 
probably be omitted (& probably also in the definition, but it's a somewhat 
separate issue/has different tradeoffs)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-11 Thread John McCall via Phabricator via cfe-commits 
rjmccall added a comment.

That's about what I would expect.  One or two extra instructions per argument 
that are trivially removed in debug builds.

I don't really see a need to post about this on Discourse, but it might be 
worth a release note.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-11 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie added a comment.

In D141381#4043985 , @fdeazeve wrote:

> In D141381#4040639 , @probinson 
> wrote:
>
>> To get data about the code size impact, people typically build some large 
>> codebase with/without their patch and compare the .text sizes. It's common 
>> to use clang itself for this experiment.
>
> I ran a couple of experiments (*), running `objdump --section-headers 
> build/bin/clang | grep text`:
>
> Debug without patch:   178,707,948 bytes
> Debug with patch:  178,785,896 bytes (+0.04%)

That does sound rather promising...

I'm guessing if we're going to add this generically for -O0 it's probably worth 
a thread on discourse for broader visibility.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-11 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits 
fdeazeve added a comment.

In D141381#4040639 , @probinson wrote:

> To get data about the code size impact, people typically build some large 
> codebase with/without their patch and compare the .text sizes. It's common to 
> use clang itself for this experiment.

I ran a couple of experiments (*), running `objdump --section-headers 
build/bin/clang | grep text`:

Debug without patch:   178,707,948 bytes
Debug with patch:  178,785,896 bytes (+0.04%)
Release without patch:  85,734,800 bytes
Release with patch: 85,734,800 bytes (no diff)

(*) Baseline commit for the compiler == d74e36572a51 
, patch 
was applied on top of it.

  The clang used in these experiments was built from commit 304838e828f9
  



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-11 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits 
fdeazeve updated this revision to Diff 488233.
fdeazeve added a comment.
Herald added subscribers: kosarev, jvesely.

Change codegen to no longer depend on -g


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/CodeGen/CGDecl.cpp
  clang/test/CodeGen/aarch64-ls64.c
  clang/test/CodeGen/atomic-arm64.c
  clang/test/CodeGenCXX/amdgcn-func-arg.cpp
  clang/test/CodeGenCXX/debug-info.cpp
  clang/test/CodeGenCXX/derived-to-base-conv.cpp
  clang/test/CodeGenCoroutines/coro-params-exp-namespace.cpp
  clang/test/CodeGenCoroutines/coro-params.cpp
  clang/test/OpenMP/for_firstprivate_codegen.cpp
  clang/test/OpenMP/parallel_firstprivate_codegen.cpp
  clang/test/OpenMP/sections_firstprivate_codegen.cpp
  clang/test/OpenMP/single_firstprivate_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_firstprivate_codegen.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_firstprivate_codegen.cpp
  
clang/test/OpenMP/target_teams_distribute_parallel_for_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/target_teams_distribute_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/teams_distribute_firstprivate_codegen.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_firstprivate_codegen.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/teams_distribute_simd_firstprivate_codegen.cpp
  clang/test/OpenMP/teams_firstprivate_codegen.cpp

Index: clang/test/OpenMP/teams_firstprivate_codegen.cpp
===
--- clang/test/OpenMP/teams_firstprivate_codegen.cpp
+++ clang/test/OpenMP/teams_firstprivate_codegen.cpp
@@ -557,8 +557,10 @@
 // CHECK9-NEXT:  entry:
 // CHECK9-NEXT:[[THIS_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:[[S_ADDR:%.*]] = alloca ptr, align 8
+// CHECK9-NEXT:[[T_INDIRECT_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:store ptr [[THIS]], ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:store ptr [[S]], ptr [[S_ADDR]], align 8
+// CHECK9-NEXT:store ptr [[T]], ptr [[T_INDIRECT_ADDR]], align 8
 // CHECK9-NEXT:[[THIS1:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:[[TMP0:%.*]] = load ptr, ptr [[S_ADDR]], align 8
 // CHECK9-NEXT:call void @_ZN1SIfEC2ERKS0_2St(ptr nonnull align 4 dereferenceable(4) [[THIS1]], ptr nonnull align 4 dereferenceable(4) [[TMP0]], ptr [[T]])
@@ -784,8 +786,10 @@
 // CHECK9-NEXT:  entry:
 // CHECK9-NEXT:[[THIS_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:[[S_ADDR:%.*]] = alloca ptr, align 8
+// CHECK9-NEXT:[[T_INDIRECT_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:store ptr [[THIS]], ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:store ptr [[S]], ptr [[S_ADDR]], align 8
+// CHECK9-NEXT:store ptr [[T]], ptr [[T_INDIRECT_ADDR]], align 8
 // CHECK9-NEXT:[[THIS1:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:[[F:%.*]] = getelementptr inbounds [[STRUCT_S:%.*]], ptr [[THIS1]], i32 0, i32 0
 // CHECK9-NEXT:[[TMP0:%.*]] = load ptr, ptr [[S_ADDR]], align 8
@@ -932,8 +936,10 @@
 // CHECK9-NEXT:  entry:
 // CHECK9-NEXT:[[THIS_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:[[S_ADDR:%.*]] = alloca ptr, align 8
+// CHECK9-NEXT:[[T_INDIRECT_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:store ptr [[THIS]], ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:store ptr [[S]], ptr [[S_ADDR]], align 8
+// CHECK9-NEXT:store ptr [[T]], ptr [[T_INDIRECT_ADDR]], align 8
 // CHECK9-NEXT:[[THIS1:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:[[TMP0:%.*]] = load ptr, ptr [[S_ADDR]], align 8
 // CHECK9-NEXT:call void @_ZN1SIiEC2ERKS0_2St(ptr nonnull align 4 dereferenceable(4) [[THIS1]], ptr nonnull align 4 dereferenceable(4) [[TMP0]], ptr [[T]])
@@ -1012,8 +1018,10 @@
 // CHECK9-NEXT:  entry:
 // CHECK9-NEXT:[[THIS_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:[[S_ADDR:%.*]] = alloca ptr, align 8
+// CHECK9-NEXT:[[T_INDIRECT_ADDR:%.*]] = alloca ptr, align 8
 // CHECK9-NEXT:store ptr [[THIS]], ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:store ptr [[S]], ptr [[S_ADDR]], align 8
+// CHECK9-NEXT:store ptr [[T]], ptr [[T_INDIRECT_ADDR]], align 8
 // CHECK9-NEXT:[[THIS1:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
 // CHECK9-NEXT:[[F:%.*]] = getelementptr inbounds [[STRUCT_S_0:%.*]], ptr [[THIS1]], i32 0, i32 0
 // CHECK9-NEXT:[[TMP0:%.*]] = load ptr, ptr [[S_ADDR]], align 8
@@ -1318,8 +1326,10 @@
 // CHECK11-NEXT:  entry:
 // CHECK11-NEXT:[[THIS_ADDR:%.*]] = alloca ptr, align 4
 // CHECK11-NEXT:[[S_ADDR:%.*]] = alloca ptr, align 4
+// CHECK11-NEXT:[[T_INDIRECT_ADDR:%.*]] = alloca ptr, align 4
 // CHECK11-NEXT:store ptr [[THIS]], ptr [[THIS_ADDR]], align 4
 // CHECK11-NEXT:store ptr [[S]], ptr [[S_ADDR]], align 4
+// CHECK11-NEXT:

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-10 Thread Paul Robinson via Phabricator via cfe-commits 
probinson added a subscriber: wolfgangp.
probinson added a comment.

Basically you're homing a pointer to the parameter, rather than the parameter 
itself, which makes sense in this context.

I suspect the code size/perf impact on -O0 will not be huge, as this sounds 
like it's only for byval-but-not-really parameters. Normal pointer/reference 
parameters will be handled as they always have been.  To get data about the 
code size impact, people typically build some large codebase with/without their 
patch and compare the .text sizes. It's common to use clang itself for this 
experiment.

I'm reminded that @wolfgangp developed a patch (which we use within Sony) to 
request lifetime extension as a codegen option. See his lightning talk 
 (I thought it was posted as a 
patch, but I don't have a reference handy). I don't think this has any direct 
relationship to your patch, but it's something that our licensees have found 
useful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-10 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits 
fdeazeve added a comment.

In D141381#4040314 , @dblaikie wrote:

> Ah, so the intent is that this causes these indirect args to be handled the 
> same way as other arguments at -O0 - placed in an alloca, eg:

Yup, that's correct!

> Though I guess it doesn't change the codegen so that the alloca is used to 
> load the value like with other values. Maybe it'd be worth changing the 
> codegen to be more similar in that way?

Note that the situation here is slightly different: loading this new alloca 
would _not_ produce the value, it would produce the address of the value 
(because the argument is indirect). In that way, I'm not sure we would gain 
anything by loading from the alloca. Does that make sense?

> though the impact on -O0 will be a question

Indeed. I'm not exactly sure what the right tradeoffs here are, but will think 
about this some more.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-10 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie added a comment.

Ah, so the intent is that this causes these indirect args to be handled the 
same way as other arguments at -O0 - placed in an alloca, eg:

  struct t1 {
t1(const t1&);
  };
  void f1(int, int);
  void f2(t1 v, int x) {
f1(3, 4);
  }

  0x0033: DW_TAG_formal_parameter
DW_AT_location(indexed (0x0) loclist = 0x0010: 
   [0x, 0x0010): DW_OP_breg5 
RDI+0
   [0x0010, 0x0020): 
DW_OP_entry_value(DW_OP_reg5 RDI))
DW_AT_name("v")
  ...
  
  0x003c: DW_TAG_formal_parameter
DW_AT_location(DW_OP_fbreg -4)
DW_AT_name("x")
  ...

Though I guess it doesn't change the codegen so that the alloca is used to load 
the value like with other values. Maybe it'd be worth changing the codegen to 
be more similar in that way? But maybe not. Don't know.

It seems plausible - though the impact on -O0 will be a question - if this 
hurts code size/perf too much (there's /some/ line there, but I don't know what 
it is), it couldn't go in -O0 and I'm not sure where it'd go. Logically `-Og` 
would make sense, but that's `-O1` which runs mem2reg anyway, so would lose the 
alloca, which doesn't help...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-10 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits 
fdeazeve added a comment.

In D141381#4040071 , @probinson wrote:

> just make sure it's not conditional on -g.

This makes sense, I'll update the patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-10 Thread Paul Robinson via Phabricator via cfe-commits 
probinson added a comment.

I should expand on my previous comment.  Extending the lifetime of these 
variables by saving the pointers into local variables, that's fine; just make 
sure it's not conditional on -g.  The only difference between -g and not -g 
should be metadata and dbg.* instructions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-10 Thread Paul Robinson via Phabricator via cfe-commits 
probinson added a comment.

This causes codegen to be different depending on whether debug-info is 
generated. Please don't do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-10 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits 
fdeazeve added inline comments.



Comment at: clang/lib/CodeGen/CGDebugInfo.h:494
+   CGBuilderTy ,
+   const bool UsePointerValue = false);
 

FWIW I used a `const` bool here to match the style already present in this class



Comment at: clang/lib/CodeGen/CGDecl.cpp:2482
+   CGM.getCodeGenOpts().hasReducedDebugInfo() 
&&
+   !CurFuncIsThunk && !NoDebugInfo;
+

this is hoisted from below


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-10 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits 
fdeazeve updated this revision to Diff 487784.
fdeazeve added a comment.

Improved comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/CodeGen/CGDecl.cpp
  clang/test/CodeGenCXX/debug-info.cpp

Index: clang/test/CodeGenCXX/debug-info.cpp
===
--- clang/test/CodeGenCXX/debug-info.cpp
+++ clang/test/CodeGenCXX/debug-info.cpp
@@ -4,7 +4,13 @@
 // CHECK: @_ZN6pr96081xE ={{.*}} global ptr null, align 8, !dbg [[X:![0-9]+]]
 
 // CHECK: define{{.*}} void @_ZN7pr147634funcENS_3fooE
-// CHECK: call void @llvm.dbg.declare({{.*}}, metadata ![[F:[0-9]+]], metadata !DIExpression())
+// CHECK-SAME: ptr noundef [[param:%.*]])
+// CHECK-NEXT: entry:
+// CHECK-NEXT:   alloca ptr, align 8
+// CHECK-NEXT:   [[param_addr_storage:%.*]] = alloca ptr, align 8
+// CHECK-NEXT:   store
+// CHECK-NEXT:   store ptr [[param]], ptr [[param_addr_storage]], align 8
+// CHECK-NEXT:   call void @llvm.dbg.declare(metadata ptr [[param_addr_storage]], metadata ![[F:[0-9]+]], metadata !DIExpression(DW_OP_deref))
 
 // !llvm.dbg.cu pulls in globals and their types first.
 // CHECK-NOT: !DIGlobalVariable(name: "c"
Index: clang/lib/CodeGen/CGDecl.cpp
===
--- clang/lib/CodeGen/CGDecl.cpp
+++ clang/lib/CodeGen/CGDecl.cpp
@@ -2475,6 +2475,12 @@
   Address AllocaPtr = Address::invalid();
   bool DoStore = false;
   bool IsScalar = hasScalarEvaluationKind(Ty);
+  bool UseIndirectDebugAddress = false;
+  // Emit debug info for param declarations in non-thunk functions.
+  const bool ShouldEmitDebugInfo = getDebugInfo() &&
+   CGM.getCodeGenOpts().hasReducedDebugInfo() &&
+   !CurFuncIsThunk && !NoDebugInfo;
+
   // If we already have a pointer to the argument, reuse the input pointer.
   if (Arg.isIndirect()) {
 // If we have a prettier pointer type at this point, bitcast to that.
@@ -2486,6 +2492,19 @@
 auto AllocaAS = CGM.getASTAllocaAddressSpace();
 auto *V = DeclPtr.getPointer();
 AllocaPtr = DeclPtr;
+
+// For truly ABI indirect arguments -- those that are not `byval` -- store
+// the address of the argument on the stack to preserve debug information.
+ABIArgInfo ArgInfo = CurFnInfo->arguments()[ArgNo - 1].info;
+if (ShouldEmitDebugInfo && ArgInfo.isIndirect())
+  UseIndirectDebugAddress = !ArgInfo.getIndirectByVal();
+if (UseIndirectDebugAddress) {
+  auto PtrTy = getContext().getPointerType(Ty);
+  AllocaPtr = CreateMemTemp(PtrTy, getContext().getTypeAlignInChars(PtrTy),
+D.getName() + ".indirect_addr");
+  EmitStoreOfScalar(V, AllocaPtr, /* Volatile */ false, PtrTy);
+}
+
 auto SrcLangAS = getLangOpts().OpenCL ? LangAS::opencl_private : AllocaAS;
 auto DestLangAS =
 getLangOpts().OpenCL ? LangAS::opencl_private : LangAS::Default;
@@ -2597,15 +2616,13 @@
 
   setAddrOfLocalVar(, DeclPtr);
 
-  // Emit debug info for param declarations in non-thunk functions.
-  if (CGDebugInfo *DI = getDebugInfo()) {
-if (CGM.getCodeGenOpts().hasReducedDebugInfo() && !CurFuncIsThunk &&
-!NoDebugInfo) {
-  llvm::DILocalVariable *DILocalVar = DI->EmitDeclareOfArgVariable(
-  , AllocaPtr.getPointer(), ArgNo, Builder);
-  if (const auto *Var = dyn_cast_or_null())
-DI->getParamDbgMappings().insert({Var, DILocalVar});
-}
+  if (ShouldEmitDebugInfo) {
+CGDebugInfo *DI = getDebugInfo();
+assert(DI);
+llvm::DILocalVariable *DILocalVar = DI->EmitDeclareOfArgVariable(
+, AllocaPtr.getPointer(), ArgNo, Builder, UseIndirectDebugAddress);
+if (const auto *Var = dyn_cast_or_null())
+  DI->getParamDbgMappings().insert({Var, DILocalVar});
   }
 
   if (D.hasAttr())
Index: clang/lib/CodeGen/CGDebugInfo.h
===
--- clang/lib/CodeGen/CGDebugInfo.h
+++ clang/lib/CodeGen/CGDebugInfo.h
@@ -488,10 +488,10 @@
 
   /// Emit call to \c llvm.dbg.declare for an argument variable
   /// declaration.
-  llvm::DILocalVariable *EmitDeclareOfArgVariable(const VarDecl *Decl,
-  llvm::Value *AI,
-  unsigned ArgNo,
-  CGBuilderTy );
+  llvm::DILocalVariable *
+  EmitDeclareOfArgVariable(const VarDecl *Decl, llvm::Value *AI, unsigned ArgNo,
+   CGBuilderTy ,
+   const bool UsePointerValue = false);
 
   /// Emit call to \c llvm.dbg.declare for the block-literal argument
   /// to a block invocation function.
Index: clang/lib/CodeGen/CGDebugInfo.cpp

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-10 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits 
fdeazeve updated this revision to Diff 487782.
fdeazeve added a comment.

Simplified condition spelling


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141381

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/CodeGen/CGDecl.cpp
  clang/test/CodeGenCXX/debug-info.cpp

Index: clang/test/CodeGenCXX/debug-info.cpp
===
--- clang/test/CodeGenCXX/debug-info.cpp
+++ clang/test/CodeGenCXX/debug-info.cpp
@@ -4,7 +4,13 @@
 // CHECK: @_ZN6pr96081xE ={{.*}} global ptr null, align 8, !dbg [[X:![0-9]+]]
 
 // CHECK: define{{.*}} void @_ZN7pr147634funcENS_3fooE
-// CHECK: call void @llvm.dbg.declare({{.*}}, metadata ![[F:[0-9]+]], metadata !DIExpression())
+// CHECK-SAME: ptr noundef [[param:%.*]])
+// CHECK-NEXT: entry:
+// CHECK-NEXT:   alloca ptr, align 8
+// CHECK-NEXT:   [[param_addr_storage:%.*]] = alloca ptr, align 8
+// CHECK-NEXT:   store
+// CHECK-NEXT:   store ptr [[param]], ptr [[param_addr_storage]], align 8
+// CHECK-NEXT:   call void @llvm.dbg.declare(metadata ptr [[param_addr_storage]], metadata ![[F:[0-9]+]], metadata !DIExpression(DW_OP_deref))
 
 // !llvm.dbg.cu pulls in globals and their types first.
 // CHECK-NOT: !DIGlobalVariable(name: "c"
Index: clang/lib/CodeGen/CGDecl.cpp
===
--- clang/lib/CodeGen/CGDecl.cpp
+++ clang/lib/CodeGen/CGDecl.cpp
@@ -2475,6 +2475,12 @@
   Address AllocaPtr = Address::invalid();
   bool DoStore = false;
   bool IsScalar = hasScalarEvaluationKind(Ty);
+  bool UseIndirectDebugAddress = false;
+  // Emit debug info for param declarations in non-thunk functions.
+  const bool ShouldEmitDebugInfo = getDebugInfo() &&
+   CGM.getCodeGenOpts().hasReducedDebugInfo() &&
+   !CurFuncIsThunk && !NoDebugInfo;
+
   // If we already have a pointer to the argument, reuse the input pointer.
   if (Arg.isIndirect()) {
 // If we have a prettier pointer type at this point, bitcast to that.
@@ -2486,6 +2492,19 @@
 auto AllocaAS = CGM.getASTAllocaAddressSpace();
 auto *V = DeclPtr.getPointer();
 AllocaPtr = DeclPtr;
+
+// For truly ABI indirect arguments, that is, they are not `byval`, store
+// the address of the argument on the stack to preserve debug information.
+ABIArgInfo ArgInfo = CurFnInfo->arguments()[ArgNo - 1].info;
+if (ShouldEmitDebugInfo && ArgInfo.isIndirect())
+  UseIndirectDebugAddress = !ArgInfo.getIndirectByVal();
+if (UseIndirectDebugAddress) {
+  auto PtrTy = getContext().getPointerType(Ty);
+  AllocaPtr = CreateMemTemp(PtrTy, getContext().getTypeAlignInChars(PtrTy),
+D.getName() + ".indirect_addr");
+  EmitStoreOfScalar(V, AllocaPtr, /* Volatile */ false, PtrTy);
+}
+
 auto SrcLangAS = getLangOpts().OpenCL ? LangAS::opencl_private : AllocaAS;
 auto DestLangAS =
 getLangOpts().OpenCL ? LangAS::opencl_private : LangAS::Default;
@@ -2597,15 +2616,13 @@
 
   setAddrOfLocalVar(, DeclPtr);
 
-  // Emit debug info for param declarations in non-thunk functions.
-  if (CGDebugInfo *DI = getDebugInfo()) {
-if (CGM.getCodeGenOpts().hasReducedDebugInfo() && !CurFuncIsThunk &&
-!NoDebugInfo) {
-  llvm::DILocalVariable *DILocalVar = DI->EmitDeclareOfArgVariable(
-  , AllocaPtr.getPointer(), ArgNo, Builder);
-  if (const auto *Var = dyn_cast_or_null())
-DI->getParamDbgMappings().insert({Var, DILocalVar});
-}
+  if (ShouldEmitDebugInfo) {
+CGDebugInfo *DI = getDebugInfo();
+assert(DI);
+llvm::DILocalVariable *DILocalVar = DI->EmitDeclareOfArgVariable(
+, AllocaPtr.getPointer(), ArgNo, Builder, UseIndirectDebugAddress);
+if (const auto *Var = dyn_cast_or_null())
+  DI->getParamDbgMappings().insert({Var, DILocalVar});
   }
 
   if (D.hasAttr())
Index: clang/lib/CodeGen/CGDebugInfo.h
===
--- clang/lib/CodeGen/CGDebugInfo.h
+++ clang/lib/CodeGen/CGDebugInfo.h
@@ -488,10 +488,10 @@
 
   /// Emit call to \c llvm.dbg.declare for an argument variable
   /// declaration.
-  llvm::DILocalVariable *EmitDeclareOfArgVariable(const VarDecl *Decl,
-  llvm::Value *AI,
-  unsigned ArgNo,
-  CGBuilderTy );
+  llvm::DILocalVariable *
+  EmitDeclareOfArgVariable(const VarDecl *Decl, llvm::Value *AI, unsigned ArgNo,
+   CGBuilderTy ,
+   const bool UsePointerValue = false);
 
   /// Emit call to \c llvm.dbg.declare for the block-literal argument
   /// to a block invocation function.
Index: clang/lib/CodeGen/CGDebugInfo.cpp

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

2023-01-10 Thread Felipe de Azevedo Piovezan via Phabricator via cfe-commits 
fdeazeve created this revision.
Herald added a subscriber: kristof.beyls.
Herald added a project: All.
fdeazeve requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

With codegen prior to this patch, truly indirect arguments -- i.e.
those that are not `byval` -- can have their debug information lost even
at O0. Because indirect arguments are passed by pointer, and this
pointer is likely placed in a register as per the function call ABI,
debug information is lost as soon as the register gets clobbered.

This patch solves the issue by storing the address of the parameter on
the stack, using a similar strategy employed when C++ references are
passed. In other words, this patch changes codegen from:

  define @foo(ptr %arg) {
 call void @llvm.dbg.declare(%arg, [...], metadata !DIExpression())

To:

  define @foo(ptr %arg) {
 %ptr_storage = alloca ptr
 store ptr %arg, ptr %ptr_storage
 call void @llvm.dbg.declare(%ptr_storage, [...], metadata 
!DIExpression(DW_OP_deref))

Some common cases where this may happen with C or C++ function calls:

1. "Big enough" trivial structures passed by value under the ARM ABI.
2. Structures that are non-trivial for the purposes of call (as per the Itanium 
ABI) when passed by value.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141381

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/CodeGen/CGDecl.cpp
  clang/test/CodeGenCXX/debug-info.cpp

Index: clang/test/CodeGenCXX/debug-info.cpp
===
--- clang/test/CodeGenCXX/debug-info.cpp
+++ clang/test/CodeGenCXX/debug-info.cpp
@@ -4,7 +4,13 @@
 // CHECK: @_ZN6pr96081xE ={{.*}} global ptr null, align 8, !dbg [[X:![0-9]+]]
 
 // CHECK: define{{.*}} void @_ZN7pr147634funcENS_3fooE
-// CHECK: call void @llvm.dbg.declare({{.*}}, metadata ![[F:[0-9]+]], metadata !DIExpression())
+// CHECK-SAME: ptr noundef [[param:%.*]])
+// CHECK-NEXT: entry:
+// CHECK-NEXT:   alloca ptr, align 8
+// CHECK-NEXT:   [[param_addr_storage:%.*]] = alloca ptr, align 8
+// CHECK-NEXT:   store
+// CHECK-NEXT:   store ptr [[param]], ptr [[param_addr_storage]], align 8
+// CHECK-NEXT:   call void @llvm.dbg.declare(metadata ptr [[param_addr_storage]], metadata ![[F:[0-9]+]], metadata !DIExpression(DW_OP_deref))
 
 // !llvm.dbg.cu pulls in globals and their types first.
 // CHECK-NOT: !DIGlobalVariable(name: "c"
Index: clang/lib/CodeGen/CGDecl.cpp
===
--- clang/lib/CodeGen/CGDecl.cpp
+++ clang/lib/CodeGen/CGDecl.cpp
@@ -2475,6 +2475,13 @@
   Address AllocaPtr = Address::invalid();
   bool DoStore = false;
   bool IsScalar = hasScalarEvaluationKind(Ty);
+  bool UseIndirectDebugAddress = false;
+  const bool ShouldEmitDebugInfo = [&] {
+// Emit debug info for param declarations in non-thunk functions.
+return getDebugInfo() && CGM.getCodeGenOpts().hasReducedDebugInfo() &&
+   !CurFuncIsThunk && !NoDebugInfo;
+  }();
+
   // If we already have a pointer to the argument, reuse the input pointer.
   if (Arg.isIndirect()) {
 // If we have a prettier pointer type at this point, bitcast to that.
@@ -2486,6 +2493,19 @@
 auto AllocaAS = CGM.getASTAllocaAddressSpace();
 auto *V = DeclPtr.getPointer();
 AllocaPtr = DeclPtr;
+
+// For truly ABI indirect arguments, that is, they are not `byval`, store
+// the address of the argument on the stack to preserve debug information.
+ABIArgInfo ArgInfo = CurFnInfo->arguments()[ArgNo - 1].info;
+if (ShouldEmitDebugInfo && ArgInfo.isIndirect())
+  UseIndirectDebugAddress = !ArgInfo.getIndirectByVal();
+if (UseIndirectDebugAddress) {
+  auto PtrTy = getContext().getPointerType(Ty);
+  AllocaPtr = CreateMemTemp(PtrTy, getContext().getTypeAlignInChars(PtrTy),
+D.getName() + ".indirect_addr");
+  EmitStoreOfScalar(V, AllocaPtr, /* Volatile */ false, PtrTy);
+}
+
 auto SrcLangAS = getLangOpts().OpenCL ? LangAS::opencl_private : AllocaAS;
 auto DestLangAS =
 getLangOpts().OpenCL ? LangAS::opencl_private : LangAS::Default;
@@ -2597,15 +2617,13 @@
 
   setAddrOfLocalVar(, DeclPtr);
 
-  // Emit debug info for param declarations in non-thunk functions.
-  if (CGDebugInfo *DI = getDebugInfo()) {
-if (CGM.getCodeGenOpts().hasReducedDebugInfo() && !CurFuncIsThunk &&
-!NoDebugInfo) {
-  llvm::DILocalVariable *DILocalVar = DI->EmitDeclareOfArgVariable(
-  , AllocaPtr.getPointer(), ArgNo, Builder);
-  if (const auto *Var = dyn_cast_or_null())
-DI->getParamDbgMappings().insert({Var, DILocalVar});
-}
+  if (ShouldEmitDebugInfo) {
+CGDebugInfo *DI = getDebugInfo();
+assert(DI);
+llvm::DILocalVariable *DILocalVar = DI->EmitDeclareOfArgVariable(
+, AllocaPtr.getPointer(), ArgNo, Builder, UseIndirectDebugAddress);
+if (const