Page MenuHomePhabricator

[DAGCombine] optimizeSetCCOfSignedTruncationCheck(): handle ule,ugt CondCodes.
ClosedPublic

Authored by lebedev.ri on Jul 18 2018, 10:01 AM.

Details

Summary

A follow-up for D49266 / rL337166.

At least one of these cases is more canonical,
so we really do have to handle it.
https://godbolt.org/g/pkzP3X
https://rise4fun.com/Alive/pQyhZZ

We won't get to these cases with I1 being -1,
as that will be constant-folded to true or false.

I'm also not sure we actually hit the 'ule' case,
but i think the worst think that could happen is that being dead code.

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Jul 18 2018, 10:01 AM

While there, a question: do we want to keep producing cmp with immediate in the unsigned case, or should we be producing movzx?
https://godbolt.org/g/yY2Gnp

I will probably turn this into post-commit review.

This is a pretty uncontroversial follow-up for my own previous patch, the alive agrees that this is correct,
and the only edge-case which you'd think we can have (-1 value) we won't get as that is constant-folded, and there is a test that checks that.

spatel accepted this revision.Jul 26 2018, 6:51 AM

LGTM.

lib/CodeGen/SelectionDAG/TargetLowering.cpp
1888–1889 ↗(On Diff #156104)

Nit: I don't think LLVM has any hard guidelines for this, but I've always felt that if any clause in an if-else sequence has braces, then all clauses should have braces for consistency. I don't see any value in omitting them in those situations, just more potential confusion/risk.

This revision is now accepted and ready to land.Jul 26 2018, 6:51 AM

LGTM.

Thank you for the review!

lib/CodeGen/SelectionDAG/TargetLowering.cpp
1888–1889 ↗(On Diff #156104)

Since in LLVM .clang-format case the {} will be on the same line (as in, not on the separate/new lines),
i think that would be fine.

This revision was automatically updated to reflect the committed changes.