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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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 ___

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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/

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

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

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

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

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

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:

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

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

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

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

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

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

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

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

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

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

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

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

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

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