Page MenuHomePhabricator

[InstSimplify] Fold degenerate abs of abs form
ClosedPublic

Authored by nikic on Sep 5 2020, 12:54 PM.

Details

Summary

This addresses the remaining issue from D87188. Due to a series of folds, we may end up with abs-of-abs represented as x == 0 ? -abs(x) : abs(x). Rather than recognizing this as a special abs pattern and doing an abs-of-abs fold on it afterwards, I'm directly folding this to one of the select operands in InstSimplify.

The general pattern falls into the "select with operand replaced" category, but that fold is not powerful enough to recognize that both hands of the select are the same for value zero.

Diff Detail

Event Timeline

nikic created this revision.Sep 5 2020, 12:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2020, 12:54 PM
nikic requested review of this revision.Sep 5 2020, 12:54 PM

It doesn't matter that the constant is 0 specifically, does it? Should work for any constant besides INT_MIN with the correct sign.

Is it worth trying to generalize this? It seems like something along the lines of SimplifyWithOpReplaced should be able to simplify this.

nikic added a comment.Sep 5 2020, 3:27 PM

It doesn't matter that the constant is 0 specifically, does it? Should work for any constant besides INT_MIN with the correct sign.

I believe this is only valid for 0 specifically, because that's where abs(X) == -abs(X). (Well, and for INT_MIN in the non-poisoning case, as both abs and neg preserve it as well.)

Is it worth trying to generalize this? It seems like something along the lines of SimplifyWithOpReplaced should be able to simplify this.

Yes, conceptually this could be handled by SimplifyWithOpReplaced. However, it requires replacing operands two levels up (in an abs that is used by a neg that is used by the select). Given the still open correctness issues from https://bugs.llvm.org/show_bug.cgi?id=47322, it seems dangerous to try and generalize it further.

efriedma accepted this revision.Sep 5 2020, 7:02 PM

LGTM

It doesn't matter that the constant is 0 specifically, does it? Should work for any constant besides INT_MIN with the correct sign.

I believe this is only valid for 0 specifically, because that's where abs(X) == -abs(X). (Well, and for INT_MIN in the non-poisoning case, as both abs and neg preserve it as well.)

Oh, right, it returns a value with the opposite sign.

Given the still open correctness issues from https://bugs.llvm.org/show_bug.cgi?id=47322, it seems dangerous to try and generalize it further.

I think it's worth doing something here eventually, but this is fine for now, sure.

This revision is now accepted and ready to land.Sep 5 2020, 7:02 PM
This revision was automatically updated to reflect the committed changes.