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

[Sriraman Tallam via Phabricator via llvm-commits]

tmsriram marked 9 inline comments as done and an inline comment as not done.
tmsriram 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);
----------------
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. 


================
Comment at: llvm/include/llvm/Transforms/Utils/ModuleUtils.h:112
+/// however not guaranteed to be unique.
+std::string getUniqueModuleId(Module *M, bool UseModuleId = false);
 
----------------
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?


================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:81
+      };
+      CachedMCSymbol = Ctx.getOrCreateSymbol(Unary(getNumber()) +
+                           Twine(Delimiter) + "BB" + Twine(Delimiter) +
----------------
MaskRay wrote:
> `std::string(getNumber(), 'a')`
Great catch!


================
Comment at: llvm/lib/MC/ELFObjectWriter.cpp:1374
+
+  case MCSymbolRefExpr::VK_SIZE:
+    if (MAI->shouldRelocateWithSymbols())
----------------
MaskRay wrote:
> R_*_SIZE is x86 specific. If you want to use it on other architectures, it will require an ABI change.
Currently guarded with a -mrelocate-with-symbols option.  Anything better we could do here?  


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

https://reviews.llvm.org/D68063