[PATCH] D33578: [DAGCombiner] use narrow load to avoid vector extract

[Nirav Dave via Phabricator via llvm-commits]

niravd added a comment.

LGTM modulo minor comment.



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14470
+
+  auto *Ld = dyn_cast<LoadSDNode>(N->getOperand(0));
+  auto *ExtIdx = dyn_cast<ConstantSDNode>(N->getOperand(1));
----------------
You should use BaseIndexOffset for Offset extraction. This may be better deferred to another patch with a TODO as well. 


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:14472
+  auto *ExtIdx = dyn_cast<ConstantSDNode>(N->getOperand(1));
+  if (!Ld || !Ld->hasOneUse() || Ld->isVolatile() || !ExtIdx)
+    return SDValue();
----------------
spatel wrote:
> niravd wrote:
> > The hasOneUse restriction seems overly conservative for most Targets and you've already the logic to deal with duplicated loads. This seems like a good place for "isSubVectorExtractFree". 
> Yes, I  agree this is conservative. If it's ok, I'd prefer to make this a TODO comment in this patch, and then I'll follow-up with that small enhancement. I want to be extra cautious with memop transforms to make sure we don't hit any perf or correctness problems.
> 
> Just removing the one-use check (without checking extract cost) will result in another 31 regression test files changing, so I need to take a close look at all those diffs.
Makes sense to me. 


https://reviews.llvm.org/D33578