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

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added a comment.

In https://reviews.llvm.org/D26149#607787, @apilipenko wrote:

> @filcab, the idea was mentioned above that we might want to match patterns other that bswap, for example rotate. Given this I'd prefer to keep bit-by-bit matching.


Fair enough. Do you have plans on doing that "soon", or is it just a "in the future, probably we'll have X"?

> As for improving the matching do you mean supporting the listed instructions as a part of the pattern? I do plan to add bswap and different extension nodes support in a follow up patch. I'd like to keep the initial patch simple though.

I'd prefer to be sure we're testing the thing we think we're testing. At the very least, we'd want to make sure there are no or instructions in most of those cases. I'd even go with "they should just load, maybe bswap, and exit the function", so most of those arm tests would have two or three check lines (at least). I know that currently we're emitting `ldrb` to load each byte. But I prefer tighter checks since we want them to fail as soon as possible.

btw, do you have numbers on how many times we end up triggering this on a specific code-base?



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4432
+
+    auto &Res = collectBitProviders(Op->getOperand(0),
+                                    /*CheckNumberOfUses=*/true);
----------------
Having both `Res` and `Result` seems confusing: "Which is the actual result?"
Can you change this name to something like `LHSBits`/`LHSProviders` or something?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4444
+  case ISD::ZERO_EXTEND: {
+    auto &Res = collectBitProviders(Op->getOperand(0),
+                                    /*CheckNumberOfUses=*/true);
----------------
`Res` vs `Result` again. If the `Res` means something other than "result", please write the full word, it might become sufficiently less ambiguous.
Otherwise, something like `Original`, `Narrow`, `Value` (not ideal), or something else should be better than `Res` for code reading.


https://reviews.llvm.org/D26149