[PATCH] D68063: Propeller: LLVM support for basic block sections

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/ModuleUtils.h:111
+/// If UseModuleId is true, we include the ModuleIdentifier string.  This is
+/// however not guaranteed to be unique.
+std::string getUniqueModuleId(Module *M, bool UseModuleId = false);
----------------
tmsriram wrote:
> tejohnson wrote:
> > What happens if unique module id is not actually unique?
> It does not make the situation any worse and we would could end up with two or more internal linkage functions having the same name.  Profile attribution would not be accurate and could cause sub-optimal performance decisions particularly if this function is hot.  Maybe we could warn about this and let the user pick another name for this. 
I think just clarify what the consequences of a collision are in the comments (would be a bigger problem if there was a possible correctness issue).


================
Comment at: llvm/include/llvm/Transforms/Utils/ModuleUtils.h:112
+/// however not guaranteed to be unique.
+std::string getUniqueModuleId(Module *M, bool UseModuleId = false);
 
----------------
tmsriram wrote:
> tejohnson wrote:
> > I don't see any calls using this new parameter in this patch - should this change be included with a different patch in the set?
> It is used in clang/lib/CodeGen/CodeGenModule.cpp where the function name is formed.  Is this ok?
Can you connect these patches by parent/child relationships in phabricator?


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

https://reviews.llvm.org/D68063