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

[Manman Ren via Phabricator via cfe-commits]

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 stability of the symbol name so it will not change if the relevant code for the block is not changing. It may not be as stable when the compiler version changes.
Using per-function ID improves the stability compared to per-module ID. @alexbdv do you have rough ideas on how much better the proposed approach is in terms of stability, comparing to per-function ID?
3> Using a compiler flag may slow down the adoption.

@dexonsmith: How can we move this forward? Do you have any other suggestion?

Thanks!
Manman



================
Comment at: clang/lib/AST/Mangle.cpp:52
+
+  // Dump the statement IR to a text stream for hasing
+  stmt->dump(strStmtStream);
----------------
Should it be AST instead of IR in the comment here?


================
Comment at: clang/lib/AST/Mangle.cpp:56
+
+  // Strip out addresses
+  char *ptr = &strStmtBuff[1];
----------------
Is this needed to have deterministic behavior?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74813