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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Sorry for showing up late, but I was looking at this code because it turns out it is miscompiling code for us. We're working on a test case, but there are really a number of basic LLVM coding convention problems here.

Craig, I hate to say this, but I don't think this should have gotten approved in its current form. It violates several clear guidelines in the LLVM coding standards, and pretty frequently. Maybe try to help folks ramping up on LLVM get their patches up to a higher code quality bar before approving?



================
Comment at: lib/Target/X86/X86FixupSFB.cpp:127
+
+std::map<unsigned, std::vector<unsigned>> PotentialBlockedMemCpy{
+    {X86::MOVUPSrm, {X86::MOVUPSmr, X86::MOVAPSmr}},
----------------
lsaba wrote:
> craig.topper wrote:
> > 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.
> I think DenseMap is less suitable since it doesn't have a static initializer constructor
But this still isn't going to be a constant. It isn't marked constexpr. This will require building a map every time the binary starts.

Please don't create globals with complex initializers in LLVM:
http://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors

Also, std::map is *really* slow...

Why not just build a function with a switch to do this mapping?

Even better, could this be generated by tablegen?


================
Comment at: lib/Target/X86/X86FixupSFB.cpp:1
+//===- X86FixupSFB.cpp - Avoid HW Store Forward Block issues -----------===//
+//
----------------
I find using *only* the acronym "SFB" everywhere really confusing. It makes it hard to discover things.

Also, the word `Fixup` doesn't really communicate much. Maybe `X86AvoidStoreForwardingBlocks`?


================
Comment at: lib/Target/X86/X86FixupSFB.cpp:104
+
+static bool isXMMLoadOpcode(unsigned Opcode) {
+  return Opcode == X86::MOVUPSrm || Opcode == X86::MOVAPSrm ||
----------------
Why can't these predicates be generated by tablegen from the instruction definitions. I really don't like have lots of tables in every pass as it creates a maintenance nightmare when adding new instructions and other issues.


================
Comment at: lib/Target/X86/X86FixupSFB.cpp:248-252
+  MachineOperand &Base = MI->getOperand(AddrOffset + X86::AddrBaseReg);
+  MachineOperand &Disp = MI->getOperand(AddrOffset + X86::AddrDisp);
+  MachineOperand &Scale = MI->getOperand(AddrOffset + X86::AddrScaleAmt);
+  MachineOperand &Index = MI->getOperand(AddrOffset + X86::AddrIndexReg);
+  MachineOperand &Segment = MI->getOperand(AddrOffset + X86::AddrSegmentReg);
----------------
It seems really weird to have getters above for some of these and then to not use them here.

If we want a better API for extracting the memory operands, we should build one that can be used everywhere (likely in X86InstrInfo.h) rather than a couple of ad-hoc ones that we end up not using in places like this. =[


================
Comment at: lib/Target/X86/X86FixupSFB.cpp:272
+// keep the core busy.
+static const unsigned LIMIT = 20;
+static SmallVector<MachineInstr *, 2>
----------------
We don't use all caps constants typically in LLVM. And we almost always expose debug flags for this kind of constant. It should also be at the top of the file and the function comment actually attached to the function rather than the constant.


================
Comment at: lib/Target/X86/X86FixupSFB.cpp:428-430
+      if (isPotentialBlockedMemCpyLd(MI.getOpcode())) {
+        int DefVR = MI.getOperand(0).getReg();
+        if (MRI->hasOneUse(DefVR))
----------------
Please use early-continue or early-exit to reduce indentation when writing LLVM code:
http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code



================
Comment at: lib/Target/X86/X86FixupSFB.cpp:444
+      }
+}
+unsigned FixupSFBPass::getRegSizeInBytes(MachineInstr *LoadInst) {
----------------
Missing vertical space here.


================
Comment at: lib/Target/X86/X86FixupSFB.cpp:464
+  int64_t LdStDelta = StDispImm - LdDispImm;
+  for (auto inst : BlockingStoresDisp) {
+    LdDisp2 = inst.first;
----------------
Please follow LLVM's variable naming conventions rather than `inst`.

Also `Inst` isn't a very idiomatic variable name. But worse, it is actively confusing. What is it? I would assume an instruction, perhaps a `MachineInstr`, but it isn't. It is a pair of int and and unsigned?? I have no idea what this variable means.


https://reviews.llvm.org/D41330