This is an archive of the discontinued LLVM Phabricator instance.

[DAG] add helper to bind memop chains; NFCI
ClosedPublic

Authored by spatel on May 29 2017, 6:52 AM.

Details

Summary

This step is just intended to reduce code duplication rather than change any functionality.

However, I noticed that PowerPC has PPCTargetLowering::spliceIntoChain(), and it seems to do almost the same thing as the x86 code. My understanding of this still isn't good, so it's not clear to me if the x86 code is overkill or if the PPC version is not going far enough. At the least, the code comment for spliceIntoChain() doesn't match the code itself: we don't actually add the old chain to the TokenFactor there?

Pasting the PPC code here for reference:

// Given the head of the old chain, ResChain, insert a token factor containing
// it and NewResChain, and make users of ResChain now be users of that token
// factor.
void PPCTargetLowering::spliceIntoChain(SDValue ResChain,
                                        SDValue NewResChain,
                                        SelectionDAG &DAG) const {
  if (!ResChain)
    return;

  SDLoc dl(NewResChain);

  SDValue TF = DAG.getNode(ISD::TokenFactor, dl, MVT::Other,
                           NewResChain, DAG.getUNDEF(MVT::Other));
  assert(TF.getNode() != NewResChain.getNode() &&
         "A new TF really is required here");

  DAG.ReplaceAllUsesOfValueWith(ResChain, TF);
  DAG.UpdateNodeOperands(TF.getNode(), ResChain, NewResChain);
}

Diff Detail

Event Timeline

spatel created this revision.May 29 2017, 6:52 AM
niravd accepted this revision.May 29 2017, 9:42 AM

Nice cleanup! LGTM modulo nits.

From my reading of the PPC backend, the PowerPC version is that the generic version is marginally improving debugability and there is one path in PPC target where the OldChain is not set and needs to be checked. Since the early return is semantically equivalent, I see no reason not to just add the initial check and remove the PPC splice operation.

include/llvm/CodeGen/SelectionDAG.h
1179

It'd be nice if the name bindChain could probably be clearer. Maybe something along the lines of "markSymettricMemoryOrderings".

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
7243

Nit: use getChain() for these two.

This revision is now accepted and ready to land.May 29 2017, 9:42 AM
spatel added inline comments.Jun 2 2017, 6:48 AM
include/llvm/CodeGen/SelectionDAG.h
1179

Does "makeEquivalentMemoryOrdering" capture the intent better?

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
7243

IIUC, getChain() gives us the chain operand rather than the chain result. When I try this, we get many test failures because, for example, the chain operand may not have exactly the 2 operands that UpdateNodeOperands() is expecting for a LoadSDNode.

spatel updated this revision to Diff 101202.Jun 2 2017, 6:52 AM

Patch updated:

  1. Change function name to "makeEquivalentMemoryOrdering()" to make intent clearer.
  2. Change 2nd param name and comments from "NewLoad" to "NewMemOp" (it is not necessarily a load).
  3. Add TODO comment for related PPC function.

Ping.

The patch was approved with the requirement to use "getChain()", but IIUC, we can't do that here. Can someone confirm if I understood that correctly?

niravd added inline comments.Jun 12 2017, 6:36 AM
include/llvm/CodeGen/SelectionDAG.h
1179

SGTM.

lib/CodeGen/SelectionDAG/SelectionDAG.cpp
7243

Right, of course. Nevermind.

This revision was automatically updated to reflect the committed changes.