[PATCH] D74813: [RFC] Add hash of block contents to function block names

dexonsmith added a comment. In D74813#2011083 , @alexbdv wrote: > @dexonsmith what regression are you referring to ? > What this change is effectively doing now is changing the numbering of the > blocks from incremental to hash-based. > So the demangle

[PATCH] D74813: [RFC] Add hash of block contents to function block names

alexbdv added a comment. @dexonsmith what regression are you referring to ? What this change is effectively doing now is changing the numbering of the blocks from incremental to hash-based. So the demangler functionality remains the same (i think) - I saw that it is ignoring the (currently incr

[PATCH] D74813: [RFC] Add hash of block contents to function block names

dexonsmith added a comment. In D74813#2010859 , @erik.pilkington wrote: > In D74813#2010767 , @alexbdv wrote: > > > @erik.pilkington would the hash-based numbering be OK for now ? > > > Feel free to drop the demang

[PATCH] D74813: [RFC] Add hash of block contents to function block names

erik.pilkington edited reviewers, added: rjmccall; removed: libc++abi. erik.pilkington added a comment. In D74813#2010767 , @alexbdv wrote: > @erik.pilkington would the hash-based numbering be OK for now ? Feel free to drop the demangler changes for now,

[PATCH] D74813: [RFC] Add hash of block contents to function block names

alexbdv added a comment. @erik.pilkington would the hash-based numbering be OK for now ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74813/new/ https://reviews.llvm.org/D74813 ___ cfe-commits mailing

[PATCH] D74813: [RFC] Add hash of block contents to function block names

alexbdv added a comment. @erik.pilkington How about just adding numeric hash of block parameters for now ? This way we don't have to change the mangling / demangling scheme at all. The mangling / demangling changes bring me into parts of LLVM that I'm not familiar with. (PS - I still have to upd

[PATCH] D74813: [RFC] Add hash of block contents to function block names

alexbdv updated this revision to Diff 259684. alexbdv marked an inline comment as done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74813/new/ https://reviews.llvm.org/D74813 Files: clang/lib/AST/Mangle.cpp Index: clang/lib/AST/Mangle.cpp

[PATCH] D74813: [RFC] Add hash of block contents to function block names

erik.pilkington added inline comments. Comment at: clang/lib/AST/Mangle.cpp:41 + for (ParmVarDecl *PVD : BD->parameters()) { +Context.mangleTypeName(PVD->getType(), Out); + } >>! In D74813#1996143, @alexbdv wrote: > @dexonsmith, @erik.pilkington - how about

[PATCH] D74813: [RFC] Add hash of block contents to function block names

jhenderson added inline comments. Comment at: llvm/test/tools/llvm-cxxfilt/invalid.test:1 -RUN: llvm-cxxfilt -n _Z1fi __Z1fi f ___ZSt1ff_block_invoke | FileCheck %s +RUN: llvm-cxxfilt -n _Z1fi __Z1fi f ___ZSt1ff_block_invoke ___Z7my_mainv_block_invoke_ZTSi_ZTSj_ZTSPVK3sss | File

[PATCH] D74813: [RFC] Add hash of block contents to function block names

alexbdv marked 6 inline comments as done. alexbdv added a comment. For tests - the existing block tests should be enough, just need to update them. I updated a few - want to make sure changes are final before updating all the tests to not have to update tests again. Comment at

[PATCH] D74813: [RFC] Add hash of block contents to function block names

alexbdv updated this revision to Diff 259487. Herald added a reviewer: jhenderson. Herald added a project: libc++abi. Herald added a subscriber: libcxx-commits. Herald added a reviewer: libc++abi. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74813/ne

[PATCH] D74813: [RFC] Add hash of block contents to function block names

erik.pilkington added a comment. The demangler changes should also be made in `libcxxabi/src/demangle`, as there is a copy of the demangler there. This change also needs tests, there should be demangler tests (in libcxxabi/test/test_demangle.cpp), and IRGen tests (in clang/test/CodeGen) that ch

[PATCH] D74813: [RFC] Add hash of block contents to function block names

alexbdv updated this revision to Diff 259181. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74813/new/ https://reviews.llvm.org/D74813 Files: clang/lib/AST/Mangle.cpp llvm/include/llvm/Demangle/ItaniumDemangle.h Index: llvm/include/llvm/Demangle/ItaniumDemangle.h ==

[PATCH] D74813: [RFC] Add hash of block contents to function block names

alexbdv added a comment. @dexonsmith, @erik.pilkington - how about this ? Z7my_mainv_block_invoke_ZTSi_ZTSj_ZTSPVK3sss demangled as: invocation function for block with params '(int, unsigned int, sss const volatile*)' in my_main() CHANGES SINCE LAST ACTION https://reviews.llvm.org/D748

[PATCH] D74813: [RFC] Add hash of block contents to function block names

dexonsmith added a comment. Yes, I like the look of that. Can you also update the demangler to reverse this nicely? @erik.pilkington can help there. There are two copies. Usually you modify the copy in `libcxxabi/` and then run a script to copy the changes into `llvm/`. Repository: rG L

[PATCH] D74813: [RFC] Add hash of block contents to function block names

alexbdv added a comment. @dexonsmith - I think that should work - like this ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74813/new/ https://reviews.llvm.org/D74813 ___ cfe-commits mailing list cfe-c

[PATCH] D74813: [RFC] Add hash of block contents to function block names

alexbdv updated this revision to Diff 257616. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74813/new/ https://reviews.llvm.org/D74813 Files: clang/lib/AST/Mangle.cpp Index: clang/lib/AST/Mangle.cpp ==

[PATCH] D74813: [RFC] Add hash of block contents to function block names

dexonsmith added a comment. In D74813#1982089 , @alexbdv wrote: > @erik.pilkington / @vsk / @dexonsmith - how block name = > hash(parent_function_name + block_type) ? > So any blocks that are inside the same parent function + have the same type > will

[PATCH] D74813: [RFC] Add hash of block contents to function block names

alexbdv added a comment. @erik.pilkington / @vsk / @dexonsmith - how block name = hash(parent_function_name + block_type) ? So any blocks that are inside the same parent function + have the same type will get an incremental number to de-duplicate names. CHANGES SINCE LAST ACTION https://rev

[PATCH] D74813: [RFC] Add hash of block contents to function block names

alexbdv updated this revision to Diff 257508. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74813/new/ https://reviews.llvm.org/D74813 Files: clang/include/clang/AST/Mangle.h clang/lib/AST/Mangle.cpp clang/lib/CodeGen/CGBlocks.cpp clang/lib/CodeGen/CGDecl.cpp clang/lib/CodeGen/

[PATCH] D74813: [RFC] Add hash of block contents to function block names

erik.pilkington added a comment. What do you think about using the block type instead of a hash? That should be stable across compiler versions, changes to the block body, and changes to referenced declarations. You'd still have to number blocks of the same type within the same function, but th

[PATCH] D74813: [RFC] Add hash of block contents to function block names

alexbdv added a comment. @dexonsmith - any suggestions to move forward on this ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74813/new/ https://reviews.llvm.org/D74813 ___ cfe-commits mailing list cf

[PATCH] D74813: [RFC] Add hash of block contents to function block names

alexbdv marked an inline comment as done. alexbdv added inline comments. Comment at: clang/lib/AST/Mangle.cpp:56 + + // Strip out addresses + char *ptr = &strStmtBuff[1]; manmanren wrote: > Is this needed to have deterministic behavior? Correct. Repository:

[PATCH] D74813: [RFC] Add hash of block contents to function block names

manmanren added a comment. There are a few concerns about the approach: 1> Compile time: dumping AST as string then hashing the string. Alex measured it on a synthetic benchmark, it shows insignificant impact. 2> Stability: from my understanding, the main goal of this patch is to increase stabil

[PATCH] D74813: [RFC] Add hash of block contents to function block names

alexbdv added a comment. @vsk / @dexonsmith - I've added some more info in latest comments. Let me know if I can provide more info / context on this to be able to reach a conclusion. Or if you think it is clear at this point that the hash-based approach is a no-go. Repository: rG LLVM Gith

[PATCH] D74813: [RFC] Add hash of block contents to function block names

alexbdv added a comment. @dexonsmith I did a benchmark with the worst case example I can come up with - 1000 regular functions and 1000 blocks : https://paste.ofcode.org/CFU6b46nuAA6ymxUEpkzka I didn't manage to measure any performance decrease (the performance decrease was within the noise of

[PATCH] D74813: [RFC] Add hash of block contents to function block names

alexbdv added a comment. @vsk - about breaking existing workflows - I was referring only to if / when this gets shipped out as the default - all the names for the function blocks will change and this might cause issue with tooling that relies on symbol names being consistent across builds. @de

[PATCH] D74813: [RFC] Add hash of block contents to function block names

dexonsmith added a comment. Thanks for working on this, I agree it's a really important problem. I'm as optimistic as Vedant that this is the right approach though. - On compile time, I do think there's reason to be concerned, since dumping IR was fairly expensive last I checked due to numberin

[PATCH] D74813: [RFC] Add hash of block contents to function block names

vsk added a comment. In D74813#1883241 , @alexbdv wrote: > As for making it default - would rather have this under a flag as hashing the > block contents does have some overhead and I imagine this feature wouldn't be > beneficial in most scenarios. Also,

[PATCH] D74813: [RFC] Add hash of block contents to function block names

alexbdv added a subscriber: vsk. alexbdv added a comment. @vsk - sure will add tests when removing from RFC. As for making it default - would rather have this under a flag as hashing the block contents does have some overhead and I imagine this feature wouldn't be beneficial in most scenarios.

[PATCH] D74813: [RFC] Add hash of block contents to function block names

vsk edited reviewers, added: ahatanak, erik.pilkington, dexonsmith; removed: vsk. vsk added a comment. At a high level the idea sounds good to me. For out OSes, symbol name instability results in serious performance regressions as this invalidates text/data orderfiles. Why shouldn’t this be on

[PATCH] D74813: [RFC] Add hash of block contents to function block names

alexbdv created this revision. alexbdv added reviewers: MaskRay, vsk, JonasToth, ruiu. Herald added a project: clang. Herald added a subscriber: cfe-commits. Function blocks don't have a name specified in source code. Currently their symbol name is based on the parent function's name and an index