[PATCH] D70487: Summary:[DAGCombiner] Allow zextended load combines.

[Clement Courbet via Phabricator via llvm-commits]

courbet marked an inline comment as done.
courbet added inline comments.


================
Comment at: llvm/test/CodeGen/ARM/load-combine-big-endian.ll:637
+; CHECK-NEXT:    lsl r0, r0, #16
+; CHECK-NEXT:    orr r0, r1, r0, lsr #24
 ; CHECK-NEXT:    mov pc, lr
----------------
dmgreen wrote:
> courbet wrote:
> > RKSimon wrote:
> > > regression
> > Yes, that's something I meant to highlight in thsi patch, but forgot to send the comment:
> > 
> > I've seen in other tests for ARM we're OK paying quite a large number of instructions to avoid loads, e.g.
> > 
> > ```
> > define i32 @load_i32_by_i8_bswap(i32* %arg) {
> > ; BSWAP is not supported by 32 bit target
> > ; CHECK-LABEL: load_i32_by_i8_bswap:
> > ; CHECK:       @ %bb.0:
> > ; CHECK-NEXT:    ldr r0, [r0]
> > ; CHECK-NEXT:    mov r1, #65280
> > ; CHECK-NEXT:    mov r2, #16711680
> > ; CHECK-NEXT:    and r1, r1, r0, lsr #8
> > ; CHECK-NEXT:    and r2, r2, r0, lsl #8
> > ; CHECK-NEXT:    orr r1, r1, r0, lsr #24
> > ; CHECK-NEXT:    orr r0, r2, r0, lsl #24
> > ; CHECK-NEXT:    orr r0, r0, r1
> > ; CHECK-NEXT:    mov pc, lr
> > ```
> > 
> > I'm not very familiar with these targets, where do we draw the line ?
> This appears to be running under a very old architecture where bswap is not legal.
> 
> Any idea why it is trying to perform this optimisation when there is no bswap to make it efficient?
```
// Before legalize we can introduce illegal bswaps which will be later
// converted to an explicit bswap sequence. This way we end up with a single
// load and byte shuffling instead of several loads and byte shuffling.
if (NeedsBswap && LegalOperations && !TLI.isOperationLegal(ISD::BSWAP, VT))
  return SDValue();
```

I can always bail out on zext:

```
if (NeedsBswap && (LegalOperations || NeedsZext) && !TLI.isOperationLegal(ISD::BSWAP, VT))
  return SDValue();
```

But it's not clear to me what's worth between a load and 4 arithmetic instructions on ARM.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70487/new/

https://reviews.llvm.org/D70487