This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ] Don't reuse a sub that can overflow during isel.
ClosedPublic

Authored by jonpa on Mar 10 2023, 10:05 AM.

Details

Summary

Don't use a possibly overflowing ISD::SUB node as a check against 0.

Removing this optimization for the cases of SUBs with the NSW/NUW flag set only gives only a negligible difference over benchmarks: ~50 more compares and no noticeable performance changes on SPEC. Removing the adjustForSubtraction() entirely however gave more changes: 1530 more compares, so it seems advisable to keep that function for the SUBs without NSW/NUW flags.

Fixes: https://github.com/llvm/llvm-project/issues/61268

Diff Detail

Event Timeline

jonpa created this revision.Mar 10 2023, 10:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2023, 10:05 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
jonpa requested review of this revision.Mar 10 2023, 10:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2023, 10:05 AM
nikic added a subscriber: nikic.Mar 11 2023, 1:43 AM
nikic added inline comments.
llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
2424

It would be better to drop the flags instead -- the general preference is to drop nowrap flags rather than block optimizations due to their presence.

jonpa added inline comments.Mar 11 2023, 2:17 AM
llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
2424

Hmm - I wish I had a better grasp of this.. How is it that we can just drop the nowrap flag and let the program behavior change and print the wrong result?

nikic added inline comments.Mar 11 2023, 2:22 AM
llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
2424

Dropping nowrap flag just means that instead of returning poison, the instruction will now return a well-defined value instead. This is always safe to do and will not change program behavior in the usual sense (this is called "refinement").

jonpa updated this revision to Diff 504716.Mar 13 2023, 9:38 AM

ok, I think I got it now: we can drop the NW flags because the backend will then later properly handle OF as well during comparison elimination (and the program prints the right result).

Patch updated to drop the flags instead, per review.

nikic accepted this revision.Mar 14 2023, 4:09 AM

LGTM

This revision is now accepted and ready to land.Mar 14 2023, 4:09 AM
jonpa added a comment.Mar 14 2023, 7:20 AM

LGTM

Thanks for review! @uweigand LGTY?

uweigand accepted this revision.Mar 14 2023, 10:30 AM

LGTM as well, thanks!