Page MenuHomePhabricator

[DAG] SimplifyDemandedBits - use KnownBits comparisons to remove ISD::UMIN/UMAX ops

Authored by RKSimon on Tue, Jan 12, 10:31 AM.



Use the KnownBits icmp comparisons to determine when a ISD::UMIN/UMAX op is unnecessary should either op be known to be ULE/UGE than the other.

Diff Detail

Event Timeline

RKSimon created this revision.Tue, Jan 12, 10:31 AM
RKSimon requested review of this revision.Tue, Jan 12, 10:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jan 12, 10:31 AM
foad added a subscriber: foad.Wed, Jan 13, 2:58 AM
foad added a comment.Wed, Jan 13, 6:22 AM

LGTM. Any reason not to do this for SMAX and SMIN too?

LGTM. Any reason not to do this for SMAX and SMIN too?

One step at a time :-) The UMIN/UMAX support will help some USUBSAT matching I'm working on, also we don't have test coverage yet and I haven't gotten around to creating SMIN/SMAX tests yet.

Anyone have any final comments - otherwise I'll commit this tomorrow.

foad added inline comments.Thu, Jan 14, 7:28 AM

I think this check can miss some opportunities because it is slightly asymmetrical. For example, with 4-bit numbers, if LHS = 110x and RHS = x100 then ule(LHS, RHS) will return None. However ult(LHS, RLHS) would return False, so you could optimise to RHS.

It does seem a shame to have to call both ule and ult just to catch this case. I wonder if there is a better way to code it.

RKSimon added inline comments.Fri, Jan 15, 1:57 AM

Yeah, it seems a shame but we don't have good alternatives - adding the ult/ugt test as well doesn't improve any additional existing tests - I can add coverage if you want though?

yubing added a subscriber: yubing.Fri, Jan 15, 3:06 AM
foad added inline comments.Fri, Jan 15, 5:35 AM

I'd appreciate it but I certainly won't insist. I think the current patch is fine as far as it goes.

RKSimon updated this revision to Diff 316935.Fri, Jan 15, 6:45 AM

add ult/ugt handling

foad accepted this revision.Fri, Jan 15, 9:08 AM

LGTM, thanks!

This revision is now accepted and ready to land.Fri, Jan 15, 9:08 AM