[PATCH] D16404: [X86] Use hash table in LEA optimization pass

[Quentin Colombet via llvm-commits]

qcolombet added inline comments.

================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:42
@@ +41,3 @@
+    DisableX86LEAOpt("disable-x86-lea-opt", cl::Hidden,
+                     cl::desc("X86: Disable LEA optimizations."),
+                     cl::init(false));
----------------
This change is unrelated and could be its own commit.

================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:59
@@ +58,3 @@
+
+// A key based on instruction's memory operands.
+class MemOpKey {
----------------
You should use /// here.

================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:83
@@ +82,3 @@
+    // Addresses' displacements must be either immediates or the same global.
+    if ((Disp->isImm() && Other.Disp->isImm()) ||
+        (Disp->isGlobal() && Other.Disp->isGlobal() &&
----------------
Explain in a comment why having two different immediate is still okay for operator==.

================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:91
@@ +90,3 @@
+
+  const MachineOperand *Operands[4];
+  const MachineOperand *Disp;
----------------
Add comments for each field.

================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:100
@@ +99,3 @@
+    size_t Hash = hash_value(*Key.Operands[0]);
+    Hash |= hash_value(*Key.Operands[1]) << 32;
+    Hash |= hash_value(*Key.Operands[2]) >> 32;
----------------
This may be undefined behavior if size_t is a 32-bit value.
In other words, promote the computation on int64_t, then convert to the size_t type.

================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:125
@@ +124,3 @@
+static inline MemOpKey getMemOpKey(const MachineInstr &MI, unsigned N) {
+  return MemOpKey(&MI.getOperand(N + X86::AddrBaseReg),
+                  &MI.getOperand(N + X86::AddrScaleAmt),
----------------
We should have an assert on the kind of MI we get as input.
E.g., adding X86::AddrDisp may not make sense.

================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:153
@@ +152,3 @@
+  typedef std::unordered_map<MemOpKey, SmallVector<MachineInstr *, 16>>
+      MemOpMap;
+
----------------
Why not use a DenseMap?


http://reviews.llvm.org/D16404