Page MenuHomePhabricator

[CodeGenPrepare] Move Extension Instructions Through Logical And Shift Instructions
ClosedPublic

Authored by Carrot on Apr 11 2018, 2:48 PM.

Details

Summary

CodeGenPrepare pass move extension instructions close to load instructions in different BB, so they can be combined later. But the extension instructions can't move through logical and shift instructions in current implementation. This patch enables this enhancement, so we can eliminate more extension instructions.

Diff Detail

Event Timeline

Carrot created this revision.Apr 11 2018, 2:48 PM
dblaikie edited reviewers, added: qcolombet; removed: dblaikie.Apr 23 2018, 12:59 PM
dblaikie added a subscriber: dblaikie.

Not really my area of expertise - just skimming the commit history for the file it looks like Quentin did a bunch of nearby work, so perhaps he can help?

Hi,

High level comment to help me dive into your changes.
See inlined.

Cheers,
Q

lib/CodeGen/CodeGenPrepare.cpp
3254

Could you add comments explaining what the new parameters are and their use?

test/Transforms/CodeGenPrepare/X86/ext-logicop.ll
2

Could you run opt instnamer on the input file first?
(To get rid of the implicit variable (%[0-9]+)

Carrot updated this revision to Diff 143654.Apr 23 2018, 2:42 PM
Carrot marked 2 inline comments as done.
Carrot added inline comments.
lib/CodeGen/CodeGenPrepare.cpp
3254

The new parameters are used to check:

  1. if the extension is free.
  2. if the following transformation is doable

    and(ext(shl(opnd, cst)), cst) --> and(shl(ext(opnd), ext(cst)), cst)
qcolombet added inline comments.Apr 23 2018, 4:03 PM
lib/CodeGen/CodeGenPrepare.cpp
3246

typo: instruction

3255

Both IsSExt and ConsideredExtType can be derived from ExtInst now.
We should get rid of those parameters.

3366

I believe this check does not belong here.
All the cost checks are done later.
Here we are just checking if it is semantically correct to move the extension up.

Carrot updated this revision to Diff 143806.Apr 24 2018, 2:20 PM
Carrot marked an inline comment as done.
Carrot added inline comments.
lib/CodeGen/CodeGenPrepare.cpp
3246

deleted.

3255

Parameter ExtInst is deleted.

3366

Removed. It directly makes parameter TLI unnecessary. Another use of ExtInst in pattern and(ext(shl(opnd, cst)), cst) can be found through its user since it must be the only user. So we can keep the interface unchanged.

qcolombet added inline comments.Apr 24 2018, 5:03 PM
lib/CodeGen/CodeGenPrepare.cpp
3369

Do we need to restrict this to constants?

3382

Same question as And and Or, do we need to restrict to constant?
More generally, I am not an expert on how we are dealing with poisioned value, but we probably want to call out (in a comment) that given we don't have the nuw flag, we may change a poisioned value into a regular value (eg., zext i32(shrl i8 %val, 12) => gives poison, but shrl i32 (zext i8 %val), 12 is totally fine. I believe this is okay because undef also covers valid value.

3386

Same comment as the previous one.

Carrot updated this revision to Diff 144006.Apr 25 2018, 2:28 PM
Carrot marked 2 inline comments as done.
Carrot added inline comments.
lib/CodeGen/CodeGenPrepare.cpp
3369

It is same as DAGCombiner::visitZERO_EXTEND. Extension of constant doesn't cause extra cost at run time, it makes sure moving extension across this instruction doesn't bring another extension on the other operand. Only in rare case the extension on the other operand can also be combined with another load.

qcolombet added inline comments.Apr 27 2018, 5:15 PM
lib/CodeGen/CodeGenPrepare.cpp
3369

Agreed. However, the check of the profitability of creating more than one extension is done later. Here we focus on answering, is it possible to extend?
So constant or not, the answer should be yes (true).

My point is, unless you see a problem with the existing cost model used later, we shouldn't prevent the promotion to get evaluated.

Carrot added inline comments.Apr 30 2018, 10:33 AM
lib/CodeGen/CodeGenPrepare.cpp
3369

Sounds reasonable.
I tried removing the constant check, there is only one failure test/CodeGen/X86/zext-logicop-shift-load.ll.

The source code is:

; Don't do the folding if the other operand isn't a constant.
define i64 @test7(i8* %data, i8 %logop) {
; CHECK-LABEL: test7:
; CHECK: movb
; CHECK-NEXT: shrb
; CHECK-NEXT: orb
; CHECK-NEXT: movzbl
; CHECK-NEXT: retq
entry:

%bf.load = load i8, i8* %data, align 4
%bf.clear = lshr i8 %bf.load, 2
%0 = or i8 %bf.clear, %logop
%1 = zext i8 %0 to i64 
ret i64 %1

}

Original result is:

movb    (%rdi), %al 
shrb    $2, %al 
orb     %sil, %al 
movzbl  %al, %eax
retq

Now the result is:

movzbl  (%rdi), %ecx
shrq    $2, %rcx
movzbl  %sil, %eax
orq     %rcx, %rax
retq

The new result isn't better than old result, neither worse. Do you think we should do transformation on this test case?

The new result isn't better than old result, neither worse. Do you think we should do transformation on this test case?

Yes. As long as it isn't worse, that means the cost model works the way we want.

Carrot updated this revision to Diff 145302.May 4 2018, 2:50 PM

Removed constant restriction.

qcolombet accepted this revision.May 7 2018, 12:49 PM

Thanks for your patience, LGTM.

This revision is now accepted and ready to land.May 7 2018, 12:49 PM
This revision was automatically updated to reflect the committed changes.

I think this may have caused PR38125