[PATCH] D41330: [X86] Reduce Store Forward Block issues in HW

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp:60
+static cl::opt<bool> DisableX86AvoidStoreForwardBlocks(
+    "disable-avoid-SFB", cl::Hidden,
+    cl::desc("X86: Disable Store Forwarding Blocks fixup."), cl::init(false));
----------------
Prefix command line option strings with "x86-"


================
Comment at: lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp:344
+  SmallVector<MachineInstr *, 2> PotentialBlockers;
+  unsigned BlockLimit = 0;
+  unsigned InspectionLimit = X86AvoidSFBInspectionLimit;
----------------
Should this be called BlockCnt instead of BlockLimit. It itself is not a limit.


================
Comment at: lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp:345
+  unsigned BlockLimit = 0;
+  unsigned InspectionLimit = X86AvoidSFBInspectionLimit;
+  for (MachineBasicBlock::iterator LI = LoadInst,
----------------
Why do we need InspectionLimit? Can't we use x86AvoidSFBInspectionLimit everywhere? Or at least make InspectionLimit const. Naively it looks like the limit might be changed in the function.


================
Comment at: lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp:346
+  unsigned InspectionLimit = X86AvoidSFBInspectionLimit;
+  for (MachineBasicBlock::iterator LI = LoadInst,
+                                   BB = LoadInst->getParent()->begin();
----------------
LI isn't a great name for this iterator. The name seems to have been chosen because it starts at LoadInst, but that's not meaningful once you walk away from LoadInst.


================
Comment at: lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp:347
+  for (MachineBasicBlock::iterator LI = LoadInst,
+                                   BB = LoadInst->getParent()->begin();
+       LI != BB; --LI) {
----------------
This won't visit the first instruction in the block. And it visits LoadInst. Is that intended?


================
Comment at: lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp:368
+      MachineBasicBlock *PMBB = *PB;
+      int PredLimit = 0;
+      for (MachineBasicBlock::reverse_iterator PMI = PMBB->rbegin(),
----------------
PredCnt?


================
Comment at: lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp:544
+            BlockedLoadsStoresPairs.push_back(
+                std::pair<MachineInstr *, MachineInstr *>(&MI, &StoreMI));
+        }
----------------
Use std::make_pair so you don't need to be explicit with the types.


================
Comment at: lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp:578
+    if (LdDisp2 < LdDisp1) {
+      int overlapDelta = LdDisp1 - LdDisp2;
+      LdDisp2 += overlapDelta;
----------------
Capitalize


================
Comment at: lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp:638
+
+  int64_t prevDisp = BlockingStoresDispSizeMap.begin()->first;
+  unsigned prevSize = BlockingStoresDispSizeMap.begin()->second;
----------------
Variables should be capitalized.


================
Comment at: lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp:697
+
+    if (BlockingStoresDispSizeMap.size() == 0)
+      continue;
----------------
BlockingStoresDispSizeMap.empty()


https://reviews.llvm.org/D41330