Page MenuHomePhabricator

[CodeGenPrepare] Move Extension Instructions Through Logical And Shift Instructions

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



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?


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


3254 ↗(On Diff #142076)

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

1 ↗(On Diff #142076)

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.
3254 ↗(On Diff #142076)

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
3246 ↗(On Diff #143654)

typo: instruction

3257 ↗(On Diff #143654)

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

3370 ↗(On Diff #143654)

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.
3246 ↗(On Diff #143654)


3257 ↗(On Diff #143654)

Parameter ExtInst is deleted.

3370 ↗(On Diff #143654)

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
3369 ↗(On Diff #143806)

Do we need to restrict this to constants?

3382 ↗(On Diff #143806)

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 ↗(On Diff #143806)

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.
3369 ↗(On Diff #143806)

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
3369 ↗(On Diff #143806)

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
3369 ↗(On Diff #143806)

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: movzbl
; CHECK-NEXT: retq

%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

Now the result is:

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

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