[PATCH] D29591: [DAGCombiner] Support {a|s}ext, {a|z|s}ext load nodes in load combine

[Filipe Cabecinhas via Phabricator via llvm-commits]

filcab added a comment.

Thanks Artur.
Can you commit the tests before, so we can easily show that there's a difference? Like in the other patch.



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4465
+    if (Index >= NarrowByteWidth)
+      return Op.getOpcode() == ISD::ZERO_EXTEND
+                 ? Optional<ByteProvider>(ByteProvider::getConstantZero())
----------------
`ANY_EXTEND` mentions that the bits are undefined. Maybe we can include it too (probably there's no difference in most cases, though)?


================
Comment at: test/CodeGen/AArch64/load-combine-big-endian.ll:377
+; (i32) p[i] | ((i32) p[i + 1] << 8) | ((i32) p[i + 2] << 16) | ((i32) p[i + 3] << 24)
+define i32 @load_i32_by_i8_base_offset_index(i8 addrspace(1)* %arg, i32 %i) {
+; CHECK-LABEL: load_i32_by_i8_base_offset_index:
----------------
Should we be doing anything to addrspace !=0 pointers?


================
Comment at: test/CodeGen/ARM/fp16-promote.ll:857
 ; CHECK-VFP: add sp, sp, #8
-; CHECK-VFP: bx lr
+; CHECK-VFP: pop
 ; CHECK-NOVFP: ldrh
----------------
This looks sketchy. Does `pop` change the PC on ARM? Did it appear but wasn't there?
If so, shouldn't `bx lr` still be there, to go back to the caller? (I'm not very familiar with ARM asm)


https://reviews.llvm.org/D29591