Page MenuHomePhabricator

[X86] Emit testl instead of testb for select condition where possible
AbandonedPublic

Authored by aivchenk on Aug 18 2017, 1:12 PM.

Details

Summary

For simple cases like:

define i32 @select_0_or_neg1_zeroext(i1 zeroext %cond) {

%sel = select i1 %cond, i32 0, i32 -1
ret i32 %sel

}

..we stopped generating "testl %edi,%edi" and started generating
"testb %dil,%dil". testl encoding is 1-byte smaller than testb,
so it is preferable.
This happens since https://reviews.llvm.org/D32273 where condition
for example's select started to look like:
t7: i8 = AssertZext t5, ValueType:ch:i1

t5: i8 = truncate t4
  t4: i32 = AssertZext t2, ValueType:ch:i8
    t2: i32,ch = CopyFromReg t0, Register:i32 %vreg0

The patch fixes that by directly checking that pattern in
X86TargetLowering::LowerSELECT. The example is taken directly
from test/CodeGen/X86/select_const.ll, so the tests are already
in place.

Diff Detail

Event Timeline

aivchenk created this revision.Aug 18 2017, 1:12 PM
spatel edited edge metadata.Aug 22 2017, 8:49 AM

I haven't looked at AssertZext before, so I'm going to learn something today. :)
I'm curious about this sequence:

      t4: i32 = AssertZext t2, ValueType:ch:i8
    t5: i8 = truncate t4
  t7: i8 = AssertZext t5, ValueType:ch:i1
t8: i1 = truncate t7
  1. If an AssertZext (t7) has a truncate (t5) source operand with no other uses, can we reverse those ops (effectively, we make the assert stronger)?
  2. This leads to another question: if an AssertZext is preceded by a weaker AssertZext, can we eliminate the weaker AssertZext?

FWIW, I drafted #1, and it solves this bug (we get the 32-bit ops you're getting here). It also creates several other diffs. At first glance, they look like x86 improvements to me, but I could use a 2nd opinion on that.
If that's a reasonable generic DAG transform, I'd prefer to do that rather than complicating the x86-specific code even more.

yeah, now I also think that I covered only a narrow case for the issue: we need more general approach.

My 2 cents on your points:

  1. If the result of AssertZext is truncated with no other uses, can we just delete that AssertZext? What value does it bring if all bits that are supposed to be zeroed just being thrown away? Or the case here would be that zero extend was e.g. to 32 bits but we truncate e.g. to 16? Could you please share the x86 test changes after your patch?
  2. I think this is the right thing to do :)

yeah, now I also think that I covered only a narrow case for the issue: we need more general approach.

My 2 cents on your points:

  1. If the result of AssertZext is truncated with no other uses, can we just delete that AssertZext?

I don't think we want to do that. IIUC, the point of the AssertZext is to preserve 'zeroext' info on values that would otherwise be lost during legalization/lowering. If we delete the AssertZext, we'll probably produce worse code?

Could you please share the x86 test changes after your patch?

Sure - let me clean up the draft patch and post it for review.

aivchenk abandoned this revision.Aug 22 2017, 2:02 PM

D37017 is a better way of fixing the issue. Hence, closing