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

[Artur Pilipenko via Phabricator via llvm-commits]

apilipenko added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4465
+    if (Index >= NarrowByteWidth)
+      return Op.getOpcode() == ISD::ZERO_EXTEND
+                 ? Optional<ByteProvider>(ByteProvider::getConstantZero())
----------------
filcab wrote:
> `ANY_EXTEND` mentions that the bits are undefined. Maybe we can include it too (probably there's no difference in most cases, though)?
calculateByteProvider can return either memory location (ByteProvider with memory location specified), zero constant (zero constant ByteProvider) or unknown (None). We can only express the origin of extend bytes for `ZERO_EXTEND`. In other cases including `ANY_EXTEND` it's unknown. 

Or you meant something different?


================
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:
----------------
filcab wrote:
> Should we be doing anything to addrspace !=0 pointers?
No, there is nothing special we should do. I removed addrspace qualifiers from the tests.


================
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
----------------
filcab wrote:
> 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)
Here is the codegen for this test case:
```
test_extractelement:
	.fnstart
@ BB#0:
	.save	{r4, r5, r11, lr}
	push	{r4, r5, r11, lr}
	.pad	#8
	sub	sp, sp, #8
	ldrd	r4, r5, [r1]
	and	r1, r2, #3
	mov	r2, sp
	orr	r1, r2, r1, lsl #1
	stm	sp, {r4, r5}
	ldrh	r1, [r1]
	strh	r1, [r0]
	add	sp, sp, #8
	pop	{r4, r5, r11, pc}
```
So, in fact we push `lr` and then pop it to `pc`. I changed the pattern matching slightly to make it more clear.


https://reviews.llvm.org/D29591