[PATCH] D27861: [DAGCombiner] Match load by bytes idiom and fold it into a single load. Attempt #2.

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4481
+  const TargetLowering &TLI = DAG.getTargetLoweringInfo();
+  if (!TLI.isOperationLegal(ISD::LOAD, VT))
+    return SDValue();
----------------
What is the effect of changing this to:
```
if (LegalOperations && !TLI.isOperationLegal(ISD::LOAD, VT))
```
Would the legalize do such a bad job of splitting poorly combined loads/bswaps?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4540
+  assert(Base && "must be set");
+  assert(Loads.size() > 0 && "must be at least one load");
+
----------------
Please make the assert messages more explanatory.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4559
+  bool NeedsBswap = DAG.getDataLayout().isBigEndian() != BigEndian;
+  if (NeedsBswap && !TLI.isOperationLegal(ISD::BSWAP, VT))
+    return SDValue();
----------------
Would this work?
```
if (NeedsBswap && LegalOperations && !TLI.isOperationLegal(ISD::BSWAP, VT))
```


================
Comment at: test/CodeGen/ARM/load-combine.ll:1
+; RUN: llc < %s -mtriple=arm-unknown | FileCheck %s
+
----------------
Additionally test with a armv6/7 cpu with REV? Same for big-endian tests


https://reviews.llvm.org/D27861