Page MenuHomePhabricator

Change the policy of ZERO_EXTEND/SIGN_EXTEND for SelectionDAG builder and legalization
ClosedPublic

Authored by Jiangning on Aug 18 2014, 7:17 PM.

Details

Reviewers
t.p.northover
Summary

Hi Tim,

In SelectionDAG builder, we simply insert ZERO_EXTEND for CopyToReg if only TLI thinks ZERO_EXTEND is free.

This is not optimal if the virtual register has multiple compare instruction users crossing basic blocks.

The operand of a compare instruction could be promoted (extended) to be a signed/unsigned value if the predicate of the compare instruction IsSigned()/IsUnsigned. If multiple compare instructions use the same operand, we will unavoidably have conflict extension.

We have two optimization opportunities here,

  1. If we know the number of signed predicate user is greater than unsigned, we prefer to use signed promotion, and we use zero promotion otherwise.
  2. Predicates EQ and NE are neither signed nor unsigned, so they can be treated as either signed or unsigned. if know the incoming value is AssertSext, we should prefer to do signed promotion.

With those two optimizations, fewer signed/zero extension instructions can be inserted, and then we can expose more opportunities to Machine CSE pass in back-end.

For Cortex-A57, the following two benchmark performance improvements are observed.

spec.cpu2006.ref.470_lbm 1862.8973 -6.85%
spec.cpu2006.ref.444_namd 1549.8160 -5.43%

Thanks,
-Jiangning

Diff Detail

Event Timeline

Jiangning updated this revision to Diff 12639.Aug 18 2014, 7:17 PM
Jiangning retitled this revision from to Change the policy of ZERO_EXTEND/SIGN_EXTEND for SelectionDAG builder and legalization.
Jiangning updated this object.
Jiangning edited the test plan for this revision. (Show Details)
Jiangning added a reviewer: t.p.northover.
Jiangning set the repository for this revision to rL LLVM.
Jiangning added a subscriber: Unknown Object (MLST).
t.p.northover edited edge metadata.Aug 19 2014, 4:29 AM

Hi Jiangning,

I think this is mostly fine. As usual, just a few nits.

Cheers.

Tim.

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
875

I think this can be generalized slightly. It looks like the check against TRUNCATE is really just guarding against us deciding to apply sign extension to (assertsext LHS, i16) when we're actually emitting an i8 setcc.

I'd suggest something like

if (OpL->getOpcode() == ISD::AssertSext &&
    cast<VTSDNode>(OpL->getOperand(1))->getVT() == NewLHS.getValueType() &&
    [...]
878–879

Since we know the node is AssertSext at this point we don't need to do it again. You can just set NewLHS to OpL and NewRHS to OpR.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
778–779

Isn't there a "V->users()" adapter so you can use a range-based loop here?

It might be neater to split this logic into a separate static function too ("getPreferredExtendForValue" or something). I can see the body of that loop growing over time (with sdiv and udiv being the obvious extensions, but probably not the only ones).

Jiangning updated this revision to Diff 12688.Aug 19 2014, 10:18 PM
Jiangning edited edge metadata.

Uploaded new patch.

Hi Tim,

Thanks for your comments! And I updated the code following your comments.

Thanks again!
-Jiangning

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
875

OK. I changed the code to be like this. But I think the only case is NewLHS and NEWRHS are TRUNCATE, so I added assertions inside the if body. Is there any other case except TRUNCATE?

878–879

Accepted and code is changed accordingly.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
778–779

Accepted and code is changed accordingly.

Hi Jiangning,

Thanks for the update. I think those asserts will be a problem though:

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
875

Yes. In principle, it could come from any DAG combine or other inference (computeKnownBits for example) that inspected the input.

The simplest example I was able to exploit was PromoteIntRes_FP_TO_XINT. This program crashes with the asserts present:

define i1 @foo(float %inl, float %inr) {
  %lval = fptosi float %inl to i8
  %rval = fptosi float %inr to i8
  %sum = icmp eq i8 %lval, %rval
  ret i1 %sum
}
Jiangning updated this revision to Diff 12694.Aug 20 2014, 2:48 AM

Hi Tim,

OK. I removed the asserts and added your small case in the test.

Thanks,
-Jiangning

t.p.northover accepted this revision.Aug 20 2014, 3:01 AM
t.p.northover edited edge metadata.

Hi Jiangning,

Thanks for updating again. I think this looks fine apart from one tiny formatting thing I spotted (sorry). Feel free to commit it with that change.

Tim.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
760–761

Formatting.

This revision is now accepted and ready to land.Aug 20 2014, 3:01 AM
Eugene.Zelenko added a subscriber: Eugene.Zelenko.

Committed in rL216066.