[PATCH] D34393: Adding code padding for performance stability - infrastructure

Zvi Rackover via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 21 14:08:17 PDT 2017


zvi added a comment.

Please update the patch with a context that contains the full file (svn diff -U99999 ...)

Can you add a short section to docs/CodeGenerator.rst explaining how code padding works? It will make it easier to review this patch and following patches, and would serve as a good reference.

It would be helpful if you uploaded the next patch in the queue to justify this patch.



================
Comment at: include/llvm/MC/MCCodePadder.h:70
+
+  const MachineLoopInfo *LI;
+
----------------
Where is this used?


================
Comment at: include/llvm/MC/MCFragment.h:44
     FT_LEB,
+	FT_Padding,
     FT_SafeSEH,
----------------
check the alignment


================
Comment at: include/llvm/MC/MCFragment.h:327
 
+class MCPaddingFragment : public MCFragment {
+  /// A mask containing all the kinds relevant to this fragment. i.e. the i'th
----------------
Please add a description for this class


================
Comment at: include/llvm/MC/MCFragment.h:338
+
+  typedef struct {
+	bool IsInitialized;
----------------
Use:
struct Foo {};

instead of:
typedef struct {} Foo;


================
Comment at: include/llvm/MC/MCFragment.h:339
+  typedef struct {
+	bool IsInitialized;
+    MCInst Inst;
----------------
check alignment


================
Comment at: include/llvm/MC/MCFragment.h:407
+  void setSize(uint64_t Value) { Size = Value; }
+  bool isInstructionInitialized() const { return InstInfo.IsInitialized; }
+
----------------
Not sure we need this method


================
Comment at: lib/MC/MCAsmBackend.cpp:21
 
-MCAsmBackend::MCAsmBackend() = default;
+MCAsmBackend::MCAsmBackend() : CodePadder(nullptr) {}
 
----------------
Avoid the need for explicit initialization and destruction by using a unique_ptr<> or a similar object.
Or even better (if possible) avoid allocating on the heap.


================
Comment at: lib/MC/MCAsmBackend.cpp:25
 
+MCCodePadder *MCAsmBackend::getCodePadder() {
+  if (CodePadder == nullptr)
----------------
is getOrCreateCodePadder a better name?


================
Comment at: lib/MC/MCAsmBackend.cpp:26
+MCCodePadder *MCAsmBackend::getCodePadder() {
+  if (CodePadder == nullptr)
+    CodePadder = createCodePadder();
----------------
Is there a way to avoid these repeating runtime checks by requiring the user to ensure createCodePadder() happends before any getCodePadder()?
If handleCodePaddingFunctionStart() is called before other callers of getCodePadder(), maybe the allocation should happen there?



================
Comment at: lib/MC/MCAsmBackend.cpp:31
+
+MCCodePadder *MCAsmBackend::createCodePadder() { return new MCCodePadder(); }
+
----------------
Is this function called anywhere other than getCodePadder? If no, i think we don't need it


================
Comment at: lib/MC/MCCodePadder.cpp:63
+  // of date data.
+  if (SealedSections.find(OS->getCurrentSectionOnly()) != SealedSections.end())
+    unsealSection(OS->getCurrentSectionOnly());
----------------
if (SealedSections.count(...)) is a bit more compact 


================
Comment at: lib/MC/MCCodePadder.cpp:195
+                                         MCAsmLayout &Layout) {
+  std::unordered_map<MCPaddingFragment *, MCPFRange>::iterator
+      JurisdictionLocation = FragmentToJurisdiction.find(Fragment);
----------------
auto


================
Comment at: lib/MC/MCCodePadder.cpp:227
+  assert(InsertionResult.second &&
+         "Insertion to FragmentToJurisdiction failed");
+  return InsertionResult.first->second;
----------------
Maybe "Fragment key already exists" would be more accurate?


================
Comment at: lib/MC/MCCodePadder.cpp:233
+                                        MCAsmLayout &Layout) {
+  std::unordered_map<MCPaddingFragment *, uint64_t>::iterator
+      MaxFragmentSizeLocation = FragmentToMaxWindowSize.find(Fragment);
----------------
auto


Repository:
  rL LLVM

https://reviews.llvm.org/D34393





More information about the llvm-commits mailing list