[PATCH] D29591: [DAGCombiner] Support {a|s}ext, {a|z|s}ext load nodes in load combine
Filipe Cabecinhas via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 6 10:31:47 PST 2017
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
More information about the llvm-commits
mailing list