[PATCH] D26149: [DAGCombiner] Match load by bytes idiom and fold it into a single load

[Simon Pilgrim via llvm-commits]

RKSimon added a comment.

Before deep diving - I don't want to start making feature creep demands here but I wonder if this should be trying to handle more general cases than just BSWAP - ROTL/ROTR for instance.



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4344
+/// value of the bit is either unknown, zero or comes from memory.
+struct BitProvider {
+  enum ProviderTy {
----------------
I don't see any reason why these types are not inside DAGCombiner::MatchLoadCombine


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4385
+  SmallVector<BitProvider, 32> Bits;
+};
+
----------------
Introducing this type seems to be almost useless - why not just use SmallVector/ArrayRef types directly?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4391
+const Optional<ValueBitProviders>
+collectBitProviders(SDValue Op, bool CheckNumberOfUses = false) {
+  Optional<ValueBitProviders> Result;
----------------
Why not move this into MatchLoadCombine as a lambda?


================
Comment at: test/CodeGen/X86/load-combine.ll:2
+; RUN: llc < %s -march=x86 | FileCheck %s
+; RUN: llc < %s -march=x86-64 | FileCheck %s --check-prefix=CHECK64
+
----------------
Please test with triples not archs and regenerate the test file using utils/update_llc_test_checks.py


================
Comment at: test/CodeGen/X86/load-combine.ll:443
+  ret i32 %20
+}
----------------
Please add i64 test cases - to see what happens on 32-bit targets especially.


https://reviews.llvm.org/D26149