[PATCH] D49200: [DAGCombine] Improve Load-Store Forwarding

[Nirav Dave via Phabricator via llvm-commits]

niravd added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12622
+static inline int numElems(EVT T) {
+  return T.isVector() ? T.getVectorNumElements() : 0;
+}
----------------
RKSimon wrote:
> Not sure if this is that useful (especially as similar helpers typically return 1 for scalars)?
I have no strong opinions on this. The only reason for the zero is 0 is to distinguish a singleton vector and it's element type. It's saves having to check if compared types are both isVector or not isVector. 


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12626
+bool DAGCombiner::getTruncatedStoreValue(StoreSDNode *ST, SDValue &Val) {
+  Val = ST->getValue();
+  EVT STType = Val.getValueType();
----------------
RKSimon wrote:
> Should you update Val even if you return false?
We can either assign Val when we consider the store, or before when we pass it in.  I have a mild preference for this way as it avoid having to check that Val is ST->getValue(). We don't need to use Val afterwards if it's false; we can replace the continue on 12724 with a "return SDValue();", but I think it's a bit cleaner looking this way.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12661
+      Val = DAG.getBitcast(LDType, Val);
+    case ISD::EXTLOAD:
+      Val = DAG.getNode(ISD::ANY_EXTEND, SDLoc(LD), LDType, Val);
----------------
RKSimon wrote:
> I think you need breaks here, else add LLVM_FALLTHROUGHs
Right. Thanks.


Repository:
  rL LLVM

https://reviews.llvm.org/D49200