This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG][X86][AArch64] Require targets to specify the promotion type when using setOperationAction Promote for INT_TO_FP and FP_TO_INT
ClosedPublic

Authored by craig.topper on Nov 30 2017, 11:51 AM.

Details

Summary

Currently the promotion for these ignores the normal getTypeToPromoteTo and instead just tries to double the element width. This is because the default behavior of getTypeToPromote to just adds 1 to the SimpleVT, which has the affect of increasing the element count while keeping the scalar size the same.

If multiple steps are required to get to a legal operation type, int_to_fp will be promoted multiple times. And fp_to_int will keep trying wider types in a loop until it finds one that works.

getTypeToPromoteTo does have the ability to query a promotion map to get the type and not do the increasing behavior. It seems better to just let the target specify the promotion type in the map explicitly instead of letting the legalizer iterate via widening.

FWIW, it's worth I think for any other vector operations that need to be promoted, we have to specify the type explicitly because the default behavior of getTypeToPromote isn't useful for vectors. The other types of promotion already require either the element count is constant or the total vector width is constant, but neither happens by incrementing the SimpleVT enum. In fact I think this code from AArch64 is wrong and untested.

setOperationAction(ISD::FREM,    MVT::v4f16, Promote);
setOperationAction(ISD::FREM,    MVT::v8f16, Promote);

Diff Detail

Event Timeline

craig.topper created this revision.Nov 30 2017, 11:51 AM

Ping * 3

@t.p.northover do the AArch64 changes look ok?

Hopeful ping though everyone is probably out for the holidays.

RKSimon accepted this revision.Dec 29 2017, 1:33 PM
RKSimon added a reviewer: fhahn.
RKSimon added a subscriber: fhahn.

Makes sense to me - @fhahn @rengolin @t.p.northover any problem with the AARCH64 changes?

This revision is now accepted and ready to land.Dec 29 2017, 1:33 PM
This revision was automatically updated to reflect the committed changes.
fhahn added a comment.Jan 2 2018, 4:03 AM

AArch64 looks good, it's now clearer what's going on from the code.