[MSP430][InstCombine][DAGCombine]Poor codegen for targets with no native shifts (2/8)

[llvmbot]

Bugzilla Link	44039
Version	trunk
OS	All
Reporter	LLVM Bugzilla Contributor
CC	@rotateright

Extended Description

A number of comparisons involving bit tests are converted into shifts by InstCombine and DAGCombine. However, shifts are expensive for most 8 and 16 bit targets with comparatively cheaper selects.

It is desirable that selects are emitted instead of shifts for these targets. The following cases were identified in TargetLowering and DAGCombine and were fixed by:

https://reviews.llvm.org/D69116
https://reviews.llvm.org/D69120
https://reviews.llvm.org/D69326
https://reviews.llvm.org/D70042

Cases in InstCombine remain to be fixed. In llvm-dev it has been suggested that these cases should be fixed by reversing the current canonicalisation. I am showing them in this and following reports:

REPORTED CASE:

Source code:

int testSExtICmp_0( int x )  // (InstCombineCasts:transformSExtICmp)
{
  return (x & 32) ? -1 : 0;
}

IR code:

define i16 @testSExtICmp_0(i16 %x) {
entry:
  %0 = shl i16 %x, 10
  %sext = ashr i16 %0, 15
  ret i16 %sext
}

MSP430 Target code:

testSExtICmp_0:
	mov.b	r12, r12
	swpb	r12
	add	r12, r12
	add	r12, r12
	swpb	r12
	sxt	r12
	rra	r12
	rra	r12
	rra	r12
	rra	r12
	rra	r12
	rra	r12
	rra	r12
	ret

AVR Target code:

testSExtICmp_0:
	lsl	r24
	rol	r25
	lsl	r24
	rol	r25
	lsl	r24
	rol	r25
	lsl	r24
	rol	r25
	lsl	r24
	rol	r25
	lsl	r24
	rol	r25
	lsl	r24
	rol	r25
	lsl	r24
	rol	r25
	lsl	r24
	rol	r25
	lsl	r24
	rol	r25
	asr	r25
	ror	r24
	asr	r25
	ror	r24
	asr	r25
	ror	r24
	asr	r25
	ror	r24
	asr	r25
	ror	r24
	asr	r25
	ror	r24
	asr	r25
	ror	r24
	asr	r25
	ror	r24
	asr	r25
	ror	r24
	asr	r25
	ror	r24
	asr	r25
	ror	r24
	asr	r25
	ror	r24
	asr	r25
	ror	r24
	asr	r25
	ror	r24
	asr	r25
	ror	r24
	ret

EXPECTED RESULT:

Source code:

int testSExtICmp_0( int x )  // (InstCombineCasts:transformSExtICmp)
{
  return (x & 32) ? -1 : 0;
}

Expected IR code:

define i16 @testSExtICmp_0(i16 %x) {
entry:
  %and = and i16 %x, 32
  %tobool = icmp ne i16 %and, 0
  %cond = sext i1 %tobool to i16
  ret i16 %cond
}

Expected MSP430 Target code:

testSExtICmp_0:
	mov	r12, r13
	mov	#-1, r12
	bit	llvm/llvm-project#404, r13
	jne	.LBB1_2
	clr	r12
.LBB1_2:
	ret

Expected AVR Target code:

testSExtICmp_0:
	mov	r18, r24
	mov	r19, r25
	andi	r18, 32
	andi	r19, 0
	ldi	r24, 0
	ldi	r25, 0
	cp	r18, r24
	cpc	r19, r25
	breq	LBB1_2
	ldi	r24, 255
	ldi	r25, 255
LBB1_2:
	ret

[benshi001]

AVR has gained great optimization on 8/16/32-bit shifts.

[llvmbot]

@llvm/issue-subscribers-backend-msp430

[rotateright]

No difference with current IR/codegen:
https://godbolt.org/z/aqjoTGM7b

[benshi001]

With your test source code

int testSExtICmp_0( int x )  // (InstCombineCasts:transformSExtICmp)
{
  return (x & 32) ? -1 : 0;
}

Here is my contrast between avr-gcc and clang-trunk: https://godbolt.org/z/oY15We9f3

[rotateright]

Thanks for the updates @benshi001.

Since you removed "AVR" from the bug title here and the related reports, I assumed that AVR codegen is now ideal or at least acceptable in all of those examples. That's why I only posted MSP430 codegen in my godbolt links. Let me know if I misunderstood.

I don't really know anything about either target, so if there's something that can be shared with MSP430 to improve its codegen, that would be great.

[benshi001]

@rotateright

Thanks for your test cases. I do think AVR is acceptable because AVR's shift optimization is mainly done by me.

But I know little about MSP430, so I can not help about MSP430.