[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:109
+         Opcode == X86::VMOVUPSZ128rm || Opcode == X86::VMOVAPSZ128rm ||
+         Opcode == X86::VMOVUPDZ128rm || Opcode == X86::VMOVAPDZ128rm;
+}
----------------
I think you need to add VMOVDQU64Z128rm/VMOVDQU32Z128rm/VMOVDQA64Z128rm/VMOVDQA32Z128rm. Those are the EVEX integer equivalents.


================
Comment at: lib/Target/X86/X86FixupSFB.cpp:116
+         Opcode == X86::VMOVUPSZ256rm || Opcode == X86::VMOVAPSZ256rm ||
+         Opcode == X86::VMOVUPDZ256rm || Opcode == X86::VMOVAPDZ256rm;
+}
----------------
Add the EVEX integer instructions.


================
Comment at: lib/Target/X86/X86FixupSFB.cpp:246
+
+// TODO: Consider expanding to other addressing modes in the future
+static bool isRelevantAddressingMode(MachineInstr *MI) {
----------------
Add a comment about what's considered a relevant addressing mode?


================
Comment at: lib/Target/X86/X86FixupSFB.cpp:254
+  int AddrOffset = getAddrOffset(MI);
+  Base = &(MI->getOperand(AddrOffset + X86::AddrBaseReg));
+  Disp = &(MI->getOperand(AddrOffset + X86::AddrDisp));
----------------
Why can't these be MachineOperand references? Why are they assigned to nullptr and then immediately overwritten?


================
Comment at: test/CodeGen/X86/fixup-sfb.ll:4
+; RUN: llc --disable-fixup-SFB < %s -mtriple=x86_64-linux | FileCheck %s --check-prefix=DISABLED
+; RUN: llc < %s -mtriple=x86_64-linux -mcpu=core-avx2 | FileCheck %s -check-prefix=CHECK-AVX2
+
----------------
Add avx512vl command line?


https://reviews.llvm.org/D41330