This is an archive of the discontinued LLVM Phabricator instance.

[EarlyCSE] Improve EarlyCSE of some absolute value cases.
ClosedPublic

Authored by craig.topper on May 17 2018, 3:54 PM.

Details

Summary

Change matchSelectPattern to return X and -X for ABS/NABS in a well defined order. Adjust EarlyCSE to account for this. Ensure the SPF result is some kind of min/max and not abs/nabs in one place in InstCombine that made me nervous.

Prevously we returned the two operands of the compare part of the abs pattern. The RHS is always going to be a 0 or -1 constant. This isn't very meaning thing to return for any one. There's also some freedom in the abs pattern as to what happens when the value is equal to 0. This freedom led to early cse failing to match when different constants were used in otherwise equivalent operations. By returning the input and its negation in a defined order we can ensure an exact match. This also makes sure both patterns use the exact same subtract instruction for the negation. I believe CSE should evebntually make this happen and properly merge the nsw/nuw flags. But I'm not familiar with CSE and what order it does things in so it seemed like it might be good to really enforce that they were the same.

Diff Detail

Event Timeline

craig.topper created this revision.May 17 2018, 3:54 PM
spatel added inline comments.May 18 2018, 7:16 AM
lib/Transforms/InstCombine/InstCombineSelect.cpp
941–942 ↗(On Diff #147404)

Can you check that in as a preliminary cleanup? I think this patch should be just about EarlyCSE.

Ideally, we'd clean this up more - put the min/max predicate over the subsequent 3 transforms too and simplify that code (can FP min/max get in here?).

Even better would be to move these folds to instsimplify. We're not creating new values here.

craig.topper added inline comments.May 18 2018, 11:35 AM
lib/Transforms/InstCombine/InstCombineSelect.cpp
941–942 ↗(On Diff #147404)

I don't see any reason FP min/max can't get here.

What simplification does the min/max check provide for the other 3 transforms?

spatel added inline comments.May 18 2018, 1:00 PM
lib/Transforms/InstCombine/InstCombineSelect.cpp
941–942 ↗(On Diff #147404)

I might've been seeing something that isn't there. I was imagining if this block was pulled out as its own function, then you'd early exit if (!isMinOrMax), and that allows rearranging the subsequent checks into some shared form.

Do you know if the corresponding FP folds are done elsewhere, or are they missing, or does something prevent those?

Remove the InstCombine change.

craig.topper retitled this revision from [ValueTracking][EarlyCSE][InstCombine] Improve EarlyCSE of some absolute value cases. to [EarlyCSE] Improve EarlyCSE of some absolute value cases..May 18 2018, 3:43 PM
spatel added inline comments.May 20 2018, 7:48 AM
lib/Analysis/ValueTracking.cpp
4577–4579

This new ABS/NABS behavior should be documented in the header comment.

lib/Transforms/Scalar/EarlyCSE.cpp
163–166

This is dead code unless we remove the ABS/NABS checks above here?

Update comment. Fix dead code.

spatel accepted this revision.May 21 2018, 7:18 AM

LGTM.

This revision is now accepted and ready to land.May 21 2018, 7:18 AM
This revision was automatically updated to reflect the committed changes.