Page MenuHomePhabricator

[AggressiveInstCombine] Add support for select instruction.
ClosedPublic

Authored by aymanmus on Jan 16 2020, 4:53 AM.

Details

Summary

Add support for select instructions in the sequences that may be optimized in the AggressiveInstCombine pass.

Diff Detail

Event Timeline

aymanmus created this revision.Jan 16 2020, 4:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2020, 4:53 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

This is certainly missing tests

aymanmus updated this revision to Diff 239069.Jan 20 2020, 4:18 AM

Somehow the test wasn't included with the differential text file.
Adding now.

Verify that the translation is correct, but I don't think this patch is safe:

define i16 @cmp_select_sext_const(i8 %a) {
  %conv = sext i8 %a to i32
  %cmp = icmp slt i32 %conv, 32768
  %cond = select i1 %cmp, i32 32768, i32 %conv
  %conv4 = trunc i32 %cond to i16
  ret i16 %conv4
}

$ opt -aggressive-instcombine agg.ll -S
define i16 @cmp_select_sext_const(i8 %a) {
  %conv = sext i8 %a to i16
  %cmp = icmp slt i16 %conv, -32768
  %cond = select i1 %cmp, i16 -32768, i16 %conv
  ret i16 %cond
}

https://rise4fun.com/Alive/JCo

aymanmus updated this revision to Diff 240418.Jan 26 2020, 12:48 AM

Fix @spatel's test case. Thanks for the review.
Involve constants in cmp operations in the MinBitWidth calculations.

nikic added a subscriber: nikic.Jan 26 2020, 1:40 AM

Is it possible to split this into a patch that adds just the select handling, and another that extends the icmp support? It seems like the select part should be testable independently, as long as you don't derive the select condition from the select operands.

aymanmus updated this revision to Diff 240431.Jan 26 2020, 5:52 AM

Following @nikic's comment:
I agree, the patch is missing some select exclusive tests.
Adding such test cases.
IMHO, there is no added value to splitting to 2 patches, the select changed are too obvious, and the compare is strongly related to the select changes.

IMHO, there is no added value to splitting to 2 patches, the select changed are too obvious, and the compare is strongly related to the select changes.

That's kind of the point: The select change is obvious and can land right away, the icmp part is trickier.

I'm not really familiar with this code, but coupling the icmp and the select handling seems like a bit of a hack: The transform is also possible if the select condition is unrelated to the trunc chain, and the transform is also possible if there is an icmp dependent on the trunc chain but not used in a select condition. I think the more principled approach would be to extend the code in https://github.com/llvm/llvm-project/blob/0e0c65264aeb6f66b6f711884c55cdbf66d975f6/llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp#L241-L256 to collect the extra use icmp.

IMHO, there is no added value to splitting to 2 patches, the select changed are too obvious, and the compare is strongly related to the select changes.

That's kind of the point: The select change is obvious and can land right away, the icmp part is trickier.

+1. Minimal patches are easier to review/debug, and we've already shown that this patch has tricky edge cases (and I don't know this code very well either). Also, please push the test file with baseline CHECKs as a preliminary commit.

aymanmus updated this revision to Diff 243762.Feb 11 2020, 1:30 AM
aymanmus retitled this revision from [AggressiveInstCombine] Add support for select instructions to [AggressiveInstCombine] Add support for select instruction..
aymanmus edited the summary of this revision. (Show Details)

Removed all cmp instruction handling to get a pure SELECT patch support.

Please upload all patches with full context (-U99999)

aymanmus updated this revision to Diff 243809.Feb 11 2020, 4:32 AM

Adding full context to patch.

lebedev.ri added inline comments.Feb 11 2020, 4:49 AM
llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp
202

Why is this being changed?

aymanmus marked an inline comment as done.Feb 11 2020, 5:01 AM
aymanmus added inline comments.
llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp
202

The max operation is not needed here.
If we reach this point of the code, then ValidBitWidth MUST be bigger than IOpBitwidth (because other wise we'll hit the "continue" at line 202.

lebedev.ri added inline comments.Feb 11 2020, 5:08 AM
llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp
202

Can you submit that as a separate change then please?

nikic accepted this revision.Feb 11 2020, 5:11 AM

LGTM

llvm/lib/Transforms/AggressiveInstCombine/TruncInstCombine.cpp
59

Style nit: Omit {} if not necessary.

202

This change looks fine, but please commit it independently as NFC.

This revision is now accepted and ready to land.Feb 11 2020, 5:11 AM
aymanmus marked 2 inline comments as done.Feb 12 2020, 3:50 AM
aymanmus updated this revision to Diff 244123.Feb 12 2020, 3:53 AM

Extract the std::max omission to a separate patch as requested (patch D74476)

This revision was automatically updated to reflect the committed changes.