This is an archive of the discontinued LLVM Phabricator instance.

[LV] Pass compare predicate to getCmpSelInstrCost.
ClosedPublic

Authored by sdesmalen on Nov 26 2021, 8:55 AM.

Details

Summary

If the condition of a select is a compare, pass its predicate to
TTI::getCmpSelInstrCost to get a more accurate cost value instead
of passing BAD_ICMP_PREDICATE.

I noticed that the commit message from D90070 had a comment about the
vectorized select predicate possibly being composed of other compares with
different predicate values, but I wasn't able to construct an example
where this was an actual issue. If this is an issue, I guess we could
add another check that the block isn't predicated for any reason.

Diff Detail

Event Timeline

sdesmalen created this revision.Nov 26 2021, 8:55 AM
sdesmalen requested review of this revision.Nov 26 2021, 8:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 26 2021, 8:55 AM

Makes sense to me, but I'm not sure what the original comment about being composed of different predicates meant. The aarch64 backend is already doing this from I in some situations, but won't if the vector and scalar types do not match.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7956–7957

Should this use the correct predicate too?

sdesmalen updated this revision to Diff 391653.Dec 3 2021, 8:09 AM

Pass predicate to getCmpSelInstrCost for ICmp/FCmp as well.

sdesmalen marked an inline comment as done.Dec 3 2021, 8:10 AM

Makes sense to me, but I'm not sure what the original comment about being composed of different predicates meant.

From what I can see, there's no tests that fail if we pass in the extra information, so even if this was the desired behaviour it isn't protected by any tests.
Without clarification of that comment, I think we should pursue the thing in code that seems most sensible.

fhahn accepted this revision.Dec 6 2021, 1:54 AM

LGTM, thanks! D90070 already mentioned that this should also be used in LV.

Makes sense to me, but I'm not sure what the original comment about being composed of different predicates meant.

From what I can see, there's no tests that fail if we pass in the extra information, so even if this was the desired behaviour it isn't protected by any tests.
Without clarification of that comment, I think we should pursue the thing in code that seems most sensible.

The comment referred to cases where there may not exist a context instruction (passed to getCmpSelInstrCost) or it may use a different predicate, but this shouldn't apply to the cases here.

This revision is now accepted and ready to land.Dec 6 2021, 1:54 AM
dmgreen accepted this revision.Dec 6 2021, 2:50 AM

Cheers

This revision was landed with ongoing or failed builds.Dec 6 2021, 3:42 AM
This revision was automatically updated to reflect the committed changes.

Thanks both!