[PATCH] D41330: [X86] Reduce Store Forward Block issues in HW
Craig Topper via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 28 12:17:10 PDT 2018
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
More information about the llvm-commits
mailing list