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

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added a comment.

Looks good in general. Please check out Simon's comments, as I'm curious about them too.



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4388
+/// expression verifies that it doesn't have uses outside of the expression.
+const Optional<ByteProvider> collectByteProvider(SDValue Op, unsigned Index,
+                                                 bool Root = false) {
----------------
Nit: We're not "collecting" any more. Maybe `{get,calculate,deduce}ByteProvider`? (Or something else)
Also fix the comment.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4559
+  bool NeedsBswap = DAG.getDataLayout().isBigEndian() != BigEndian;
+  if (NeedsBswap && !TLI.isOperationLegal(ISD::BSWAP, VT))
+    return SDValue();
----------------
RKSimon wrote:
> Would this work?
> ```
> if (NeedsBswap && LegalOperations && !TLI.isOperationLegal(ISD::BSWAP, VT))
> ```
I wonder if it's useful to generate a bswap only to change it back later. Do you have an example of something llvm already does? Or would this be a future optimization possibility?


https://reviews.llvm.org/D27861