[PATCH] D60294: [DAGCombiner] [CodeGenPrepare] Split large offsets from base addresses

[Luís Marques via Phabricator via llvm-commits]

luismarques added inline comments.


================
Comment at: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:1076
+
+  for (SDNode *Node : N0->uses()) {
+    auto LoadStore = dyn_cast<MemSDNode>(Node);
----------------
craig.topper wrote:
> Doing some archaelogy.
> 
> Should this be checking the uses of the `(add, (add, x, offset1), offset2))` expression? It seems to be checking the uses of `(add x, offset1)`.
> It seems to be checking the uses of `(add x, offset1)`.

Yes, you're right.

> Should this be checking the uses of the `(add, (add, x, offset1), offset2))` expression?

Yes, I think that would be the actually correct thing to do. It seems that, because the `(add x, offset1)` was also being used in the loads/stores, in practice this was working well enough for the test coverage we had (not sure about the real world). In fact, I think if you change it to check the uses of the proper value you won't see any changes at all for all CodeGen tests, for all targets. If you're planning to fix this, can you see if you can add a test case that shows the difference in behavior?

Thanks for spotting this issue, Craig! Sorry about the bug.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60294