[PATCH] D35014: [X86] Improvement in CodeGen instruction selection for LEAs.

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

A few minor comments @lsaba @craig.topper any final comments?



================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:1178
+  if ((!EnableAggressiveFolding && Depth > 5) ||
+        (EnableAggressiveFolding && Depth > 8) )
     return matchAddressBase(N, AM);
----------------
jbhateja wrote:
> RKSimon wrote:
> > RKSimon wrote:
> > > Two hard coded depths like this is weird - better to have a getMaxOperandFoldingDepth() helper?
> > My comment still stands - try to avoid hard coded values embedded in the source - add a getMaxOperandFoldingDepth() helper.
> Helper added.
I meant make this a class method, but if you don't want to you can leave it here as lambda


================
Comment at: lib/Target/X86/X86ISelDAGToDAG.cpp:203
+
+    bool getAggressiveOperandFolding() { return OptForAggressingFolding; }
+
----------------
Make this a const method


================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:189
+static inline MemOpKey getMemOpCSEKey(const MachineInstr &MI, unsigned N) {
+  static MachineOperand DummyScale = MachineOperand::CreateImm(1);
+  assert((isLEA(MI) || MI.mayLoadOrStore()) &&
----------------
RKSimon wrote:
> Can we avoid the static?
Again, can we avoid the static?


================
Comment at: lib/Target/X86/X86OptimizeLEAs.cpp:108
+    for (int i = 0; i < 4; i++) {
+      bool copyLike = isDefCopyLike(MRI, *Operands[i]);
+      if (copyLike && !isIdenticalMI(MRI, *Operands[i], *Other.Operands[i]))
----------------
CopyLike


https://reviews.llvm.org/D35014