[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/X86FixupSFB.cpp:127
+
+std::map<unsigned, std::vector<unsigned>> PotentialBlockedMemCpy{
+    {X86::MOVUPSrm, {X86::MOVUPSmr, X86::MOVAPSmr}},
----------------
craig.topper wrote:
> Should these be DenseMaps? @RKSimon, what do you think?
> 
> Can you use a std::array or std::pair instead of std::vector for the second type since the size is fixed. Each std::vector will be a separate heap allocation.
I don't think the DenseMap question here was answered.


================
Comment at: lib/Target/X86/X86FixupSFB.cpp:232
+
+static MachineOperand *getBaseOperand(MachineInstr *MI) {
+  int AddrOffset = getAddrOffset(MI);
----------------
Return a reference?


================
Comment at: lib/Target/X86/X86FixupSFB.cpp:237
+
+static MachineOperand *getDispOperand(MachineInstr *MI) {
+  int AddrOffset = getAddrOffset(MI);
----------------
Return a reference here too.


================
Comment at: lib/Target/X86/X86FixupSFB.cpp:425
+
+void FixupSFBPass::findPotentialylBlockedCopies(MachineFunction &MF) {
+  for (auto &MBB : MF)
----------------
Potentially is mispelled.


================
Comment at: lib/Target/X86/X86FixupSFB.cpp:449
+             *LoadInst->getParent()->getParent())) /
+         8;
+}
----------------
Can you clang-format at least this function? That 8 looks really out of place. So does the splitting of the getRegClass call.


================
Comment at: lib/Target/X86/X86FixupSFB.cpp:454
+    MachineInstr *LoadInst, MachineInstr *StoreInst,
+    std::map<int64_t, unsigned> *BlockingStoresDisp) {
+  int64_t LdDispImm = getDispOperand(LoadInst)->getImm();
----------------
Pass the map by reference. Probably const reference if you're not modifying it.


https://reviews.llvm.org/D41330