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

[Filipe Cabecinhas via llvm-commits]

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:
load
bswap if needed
"ret-ish" instruction

Or:
not bswap/rev
load
shift
or

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).


https://reviews.llvm.org/D26149