[PATCH] D24430: [RFC] [DAGCombine] Better store-to-load forwarding

[Hal Finkel via llvm-commits]

hfinkel created this revision.
hfinkel added a subscriber: llvm-commits.
Herald added a subscriber: mcrosier.

This is a patch/RFC. DAGCombiner::visitLOAD currently has this code:

  // If this load is directly stored, replace the load value with the stored
  // value.
  // TODO: Handle store large -> read small portion.
  // TODO: Handle TRUNCSTORE/LOADEXT
  if (ISD::isNormalLoad(N) && !LD->isVolatile()) {
    if (ISD::isNON_TRUNCStore(Chain.getNode())) {
      StoreSDNode *PrevST = cast<StoreSDNode>(Chain);
      if (PrevST->getBasePtr() == Ptr &&
          PrevST->getValue().getValueType() == N->getValueType(0))
      return CombineTo(N, Chain.getOperand(1), Chain);
    }
  }

And so, I wondered, what might fixing these TODOs look like? Furthermore, I'm not even sure we want to do this. Improving this turns out not to be the right way to fix the problem at which I was looking. The code might nevertheless be useful. A couple observations:

 1. We can't really fix this without some kind of dead-store elimination later in the pipeline (which we don't currently have). Why? If we remove a load in favor of some other arithmetic (shifts, masks, sign extensions, etc.), but keep the (now unused) store, we often increase code size by performing the forwarding (perhaps with little resulting benefit, depending on the architecture).

 2. Our practice of canonicalizing addressing offsets using 'or' instead of 'add' when we know the addends have disjoint set bits is not uniformly handled well in DAGCombine. A lot of code does not even look for the 'or' case, such as in BaseIndexOffset::match and in FindBaseOffset. We also don't fold "(A|c1)+c2 => A+(c1+c2)" where A and c1 have no common bits. The patch includes fixes for these things. Our address selection code, in X86DAGToDAGISel::matchAddressRecursively for example, recurses through these to find the total accumulated offset, an so I suspect that we don't often notice.

This patch does not have any test-case updates, but around 50 regression tests would need to be updated.

Thoughts?

https://reviews.llvm.org/D24430

Files:
  lib/CodeGen/SelectionDAG/DAGCombiner.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D24430.70901.patch
Type: text/x-patch
Size: 28709 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160909/554d1189/attachment.bin>