[PATCH] D73608: [X86] Alternative implementation of D73606 matchAdd: don't fold a large offset into a %rip relative address

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Thanks for the patch! I only get some nits.

> This passes the test case from D73606 <https://reviews.llvm.org/D73606>.

Be a bit more explicit and just mention `fold-add-pcrel.s`



================
Comment at: llvm/lib/Target/X86/X86ISelDAGToDAG.cpp:1416
+  // already matched a displacement and the caller just added the symbolic
+  // displacement with an offset of 0. So recheck everyhing if Val is non-zero.
+  if (Val == 0)
----------------
everying -> everything

`int64_t Val = AM.Disp + Offset;` can be placed immediately below the comment.


================
Comment at: llvm/lib/Target/X86/X86ISelDAGToDAG.cpp:1589
+
+  // Try again after commuting the operands.
+  if (!matchAddressRecursively(Handle.getValue().getOperand(1), AM, Depth+1) &&
----------------
commuting -> commutating


================
Comment at: llvm/lib/Target/X86/X86ISelDAGToDAG.cpp:1590
+  // Try again after commuting the operands.
+  if (!matchAddressRecursively(Handle.getValue().getOperand(1), AM, Depth+1) &&
+      !matchAddressRecursively(Handle.getValue().getOperand(0), AM, Depth+1))
----------------
clang-format will prefer `Depth + 1`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73608/new/

https://reviews.llvm.org/D73608