[PATCH] D27861: [DAGCombiner] Match load by bytes idiom and fold it into a single load. Attempt #2.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Sorry I didn't see this (much) sooner!



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4393-4395
+/// Because the parts of the expression are not allowed to have more than one
+/// use this function iterates over trees, not DAGs. So it never visits the same
+/// node more than once.
----------------
I agree this doesn't need a set because it is tree structured, but other similar routines at the SDAG layer bound recursions. See for example computeKnownBits. I suspect this code should do something similar.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4446
+    return Index >= NarrowByteWidth
+               ? ByteProvider::getConstantZero()
+               : calculateByteProvider(NarrowOp, Index);
----------------
So, one path of what this is doing is essentially computing known-zero bits. I wonder if we should really be hand rolling that or whether we should instead use computeKnownBits. We could likely use computeKnownBits in the OR logic and then only handle recursing to find loads in the rest of these paths... Thoughts? maybe not worth it, hard to tell.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4485-4492
+  if (VT != MVT::i16 && VT != MVT::i32 && VT != MVT::i64)
+    return SDValue();
+  unsigned ByteWidth = VT.getSizeInBits() / 8;
+
+  // There is nothing to do here if the target can't load a value of this type
+  const TargetLowering &TLI = DAG.getTargetLoweringInfo();
+  if (LegalOperations && !TLI.isOperationLegal(ISD::LOAD, VT))
----------------
Why only do the legality check when in the legalize phase? When would we want to combine loads to a non-legal integer type?

Is the goal here to combine loads into a too-wide illegal integer type to let the legalizer then split them into legal sized chunks? If so, that at least needs a comment. And in that case, why only up to 64-bit integers?

Alternatively, you could do the legality check in all phases and just never combine stores when the merged value is wider than the legal integer size.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4573-4577
+  // Check that a load of the wide type is both allowed and fast on the target
+  bool Fast = false;
+  bool Allowed = TLI.allowsMemoryAccess(*DAG.getContext(), DAG.getDataLayout(),
+                                        VT, FirstLoad->getAddressSpace(),
+                                        FirstLoad->getAlignment(), &Fast);
----------------
Shouldn't this check come first, up with the legalization stuff?


https://reviews.llvm.org/D27861