This is an archive of the discontinued LLVM Phabricator instance.

[SimplifyIndVar] Canonicalize comparisons to unsigned while eliminating truncs
ClosedPublic

Authored by mkazantsev on Jun 29 2018, 4:21 AM.

Details

Summary

This is a follow-up for the patch rL335020. When we replace compares against
trunc with compares against wide IV, we can also replace signed predicates with
unsigned where it is legal.

Diff Detail

Event Timeline

mkazantsev created this revision.Jun 29 2018, 4:21 AM
mkazantsev planned changes to this revision.Jun 29 2018, 4:30 AM
greened added inline comments.Jun 29 2018, 9:22 AM
lib/Transforms/Utils/SimplifyIndVar.cpp
558

That name seems misleading. It's not checking if they match, it's checking if the higest bit is zero for both. Did you mean to add a check for both being negative as well? The comments below seem to indicate so.

greened added inline comments.Jun 29 2018, 9:23 AM
lib/Transforms/Utils/SimplifyIndVar.cpp
558

Why does this need to be a lambda?

563

Does this need to be a lambda? Seems unnecessarily complicated.

mkazantsev added inline comments.Jul 1 2018, 7:28 PM
lib/Transforms/Utils/SimplifyIndVar.cpp
558

Yes, actually in the initial version it also checked negative, but then I realized that for negative values zext(trunc(x)) is never equal to x, so in practice this case will never fire. I will change the naming here.

563

It is the simplest way to avoid something like if (Cond1 || (!Cond2 && Cond3 && Cond4)) which is harder to understand.

mkazantsev added inline comments.Jul 1 2018, 7:46 PM
lib/Transforms/Utils/SimplifyIndVar.cpp
558

It doesn't, I'll merge it with the next one.

mkazantsev updated this revision to Diff 153662.Jul 1 2018, 7:50 PM
mkazantsev marked 2 inline comments as done.

Added some comments, removed one unnecessary lambda.

LGTM.

lib/Transforms/Utils/SimplifyIndVar.cpp
563

Actually, I was wondering if it should be a freestanding function, but thinking about it now it seems like a good idea to define it where it's used. It's acting like a nested function in that sense, so I think this is fine. Thanks.

reames accepted this revision.Jul 20 2018, 3:31 PM

Marking as accepted since LGTMed by previous reviewer. Just getting it off my queue.

This revision is now accepted and ready to land.Jul 20 2018, 3:31 PM
This revision was automatically updated to reflect the committed changes.