[PATCH] D26149: [DAGCombiner] Match load by bytes idiom and fold it into a single load

Filipe Cabecinhas via llvm-commits llvm-commits at lists.llvm.org
Tue Nov 22 10:22:48 PST 2016

filcab added a comment.

This is low-level enough that it's probably worth it to just do it byte by byte, instead of bit for bit. At least for now, no?

Your pattern matching on the non-x86 cases needs to be improved. At the very least, we need to match:
bswap if needed
"ret-ish" instruction

not bswap/rev

Ideally, you'd use the same kinds of checks as the x86 versions.

Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4434
+    P.erase(std::prev(P.end(), BitShift), P.end());
+    P.insert(P.begin(), BitShift, BitProvider::getZero());
+    return P;
Why copy the whole vector only to replace parts of it?

Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4448
+    for (unsigned i = NarrowBitWidth; i < BitWidth; i++)
+      Result[i] = BitProvider::getZero();
Maybe use `std::fill_n`, `std::copy`?

Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4492
+SDValue DAGCombiner::MatchLoadCombine(SDNode *N) {
+  assert(N->getOpcode() == ISD::OR && "must be OR");
Can you make the string more explicit? Something like "Can only match load combining against OR nodes" or something?

Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4591
+                                        VT, FirstLoad->getAddressSpace(),
+                                        FirstLoad->getAlignment(), &Fast);
+  if (!Allowed || !Fast)
Should we check this before we do all the work with the bitoffsets?
Might be hard as we probably need address space + alignment info. But we can at least bubble this up to above the little/big endian decision (unsure if that's very profitable, though).


More information about the llvm-commits mailing list