[PATCH] D121355: [SelectionDAG] Fold shift constants into cmp

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

I'm not sure I like this patch as is.

The fundamental problem isn't directly the shift code. We have conflict between canonicalization and target lowering. Canonicalization prefers to create SET(U)GT/SET(U)LT. But that may create an unsupported constant. Is it the target's responsibility to detect reverse the canonicalization during lowering? We do that already for setgt X, -1 on many targets. ARM and AArch64 do it for even more constant values. Or should canonicalization be consulting isLegalICmpImmediate?

If it's the target's responsibility during, then we should just block the shift transform and leave the unsupported immediate so the target can fix it.
If canonicalization in this code should deal with it, then we should be converting SETGT/SETUGT to SETGE/SETUGE based on isLegalICmpImmediate around line 4363 in the original code. The same idea as the changes in this patch to the SETGE/SETUGE canonicalization.

What we have a right now is somewhere in the middle of these two ideas. We're only considering very specific constants by creating the constant the shift code. But we've made the SETGE/SETUGE canonicalization block transforming a potentially larger set of the constants.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121355/new/

https://reviews.llvm.org/D121355