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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

A few comments after a quick high level scan of patch. Also - please mark all these related patches as parent and child versions in Phabricator.



================
Comment at: llvm/include/llvm/CodeGen/MachineBasicBlock.h:139
 
+  MCSymbol *EndMCSymbol = nullptr;
+
----------------
comment


================
Comment at: llvm/include/llvm/CodeGen/MachineBasicBlock.h:820
+  void setEndMCSymbol(MCSymbol *Sym)
+  { EndMCSymbol = Sym; }
+
----------------
please run clang-format on this patch


================
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);
----------------
What happens if unique module id is not actually unique?


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


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2988
+    // Emit alignment after section is created for basic block sections.
+    // if (MBB.isUniqueSection()) {
+    //  if (unsigned LogAlign = MBB.getLogAlignment())
----------------
Should this be uncommented, or removed?


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:3128
 
-
   // We always return true as we have no way to track whether the final order
----------------
Only a whitespace change in this file, remove from patch.


================
Comment at: llvm/lib/MC/MCDwarf.cpp:1481
 
+static bool ShouldBeDeduped(const MCCFIInstruction &Instr,
+                            const MCSymbol *BaseLabel,
----------------
add comment summarizing when dedup is needed


================
Comment at: llvm/lib/MC/MCDwarf.cpp:1711
         MAI->getInitialFrameState();
-    EmitCFIInstructions(Instructions, nullptr);
+    EmitCFIInstructions(Instructions, nullptr, true, true);
+    EmitCFIInstructions(Frame.Instructions, Frame.Begin, true, false);
----------------
Add parameter name constants to constant parameters here and in other calls to this function.


================
Comment at: llvm/lib/Target/X86/X86FrameLowering.cpp:465
+  MachineFunction &MF = *MBB.getParent();
+  if (hasFP(MF)) {
+    const MachineModuleInfo &MMI = MF.getMMI();
----------------
change to early return. Also a comment would be good


================
Comment at: llvm/lib/Transforms/Utils/ModuleUtils.cpp:274
 
-  if (!ExportsSymbols)
-    return "";
+  // Using only module id is not guaranteed to keep this unique. So we augment
+  // the hash with the module id. This also handles the case when we have
----------------
Confusing comment, as the first 2 sentences seem contradictory (module id not guaranteed to be unique, so we use it).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68063