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

Some minor nits: No need to size the vectors at 32, 4 will do it.
And use `auto` if you're declaring a var and initializing it to the return of a `..._cast` operator, since you already mentioned the type in that line.

Please run the ASan test before pushing the commit.



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4389
+/// expression verifies that it doesn't have uses outside of the expression.
+const Optional<SmallVector<ByteProvider, 32> >
+collectByteProviders(SDValue Op, bool CheckNumberOfUses = false) {
----------------
We can make this `4` now, no?
Making it `8` would save us a `realloc()` when compiling for 64-bit archs on a bunch of places, but I'm unsure it's worth it. Something to be tried later, though.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4397
+    return None;
+  unsigned ByteWidth = Op.getScalarValueSizeInBits() / 8;
+
----------------
You already had `BitWidth`. Using it makes this line more readable.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4401
+  case ISD::OR: {
+    auto &LHS = collectByteProviders(Op->getOperand(0),
+                                     /*CheckNumberOfUses=*/true);
----------------
Aren't you binding to the object returned in `collectByteProviders`? Can you run a test case that triggers this optimization under asan with `ASAN_OPTIONS=detect_stack_use_after_return=true`?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4423
+
+    SmallVector<ByteProvider, 32> Result(ByteWidth);
+    for (unsigned i = 0; i < LHS->size(); i++)
----------------
`4`


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4430
+  case ISD::SHL: {
+    ConstantSDNode *ShiftOp = dyn_cast<ConstantSDNode>(Op->getOperand(1));
+    if (!ShiftOp)
----------------
`auto ShiftOp = ...`


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4444
+
+    SmallVector<ByteProvider, 32> Result;
+    Result.insert(Result.begin(), ByteShift, ByteProvider::getZero());
----------------
`4`


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4457
+
+    SmallVector<ByteProvider, 32> Result;
+    unsigned NarrowByteWidth = Original->size();
----------------
`4`


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4466
+  case ISD::LOAD: {
+    LoadSDNode *L = cast<LoadSDNode>(Op.getNode());
+    if (L->isVolatile() || L->isIndexed() ||
----------------
`auto L = ...`


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4474
+
+    SmallVector<ByteProvider, 32> Result(ByteWidth);
+    for (unsigned i = 0; i < ByteWidth; i++)
----------------
`4`


https://reviews.llvm.org/D26149