This is an archive of the discontinued LLVM Phabricator instance.

[X86] Improve unsigned saturation downconvert detection.
ClosedPublic

Authored by ArturGainullin on Apr 5 2018, 5:52 AM.

Details

Summary

New unsigned saturation downconvert patterns detection was implemented in
X86 Codegen:

(truncate (smin (smax (x, C1), C2)) to dest_type),
where C1 >= 0 and C2 is unsigned max of destination type.

(truncate (smax (smin (x, C2), C1)) to dest_type)
where C1 >= 0, C2 is unsigned max of destination type and C1 <= C2.

These two patterns are equivalent to:

(truncate (umin (smax(x, C1), unsigned_max_of_dest_type)) to dest_type)

Diff Detail

Event Timeline

ArturGainullin created this revision.Apr 5 2018, 5:52 AM
RKSimon added inline comments.Apr 5 2018, 9:40 AM
lib/Target/X86/X86ISelLowering.cpp
34572

Don't you need to test both SMIN(SMAX(X, C1), C2) and SMAX(SMIN(X, C2), C1) - see detectSSatPattern

  • Added detection of the smax(smin) pattern.
ArturGainullin edited the summary of this revision. (Show Details)Apr 12 2018, 3:33 PM
ArturGainullin edited the summary of this revision. (Show Details)
RKSimon added inline comments.Apr 14 2018, 4:41 AM
test/CodeGen/X86/avx512-trunc.ll
769

Please can you commit these avx512-trunc.ll tests with trunk's current codegen and then rebase so this patch shows the codegen diff?

ArturGainullin edited the summary of this revision. (Show Details)
Rebased.

Sorry, forgot to commit one of the tests. WIll fix.

Committed the test. Rebased.
RKSimon added inline comments.Apr 30 2018, 12:02 PM
lib/Target/X86/X86ISelLowering.cpp
34652

Not sure what is going on here - why do you need to create a node - it looks like this is just smax again - and there shouldn't be a difference in value types at all?

(truncate (smin (smax (x, C1), C2)) to dest_type)
is equivalent to
(truncate (umin (smax(x, C1), unsigned_max_of_dest_type)) to dest_type)
that is why I don't need to create any node and just return smax.

But in case of
(truncate (smax (smin (x, C2), C1)) to dest_type)
which is also equivalent to
(truncate (umin (smax(x, C1), unsigned_max_of_dest_type)) to dest_type)
I need to create new smax node and return it.

Simon, could you please write if my comment is not enough and you need more details or clarification.

  • Sorry, but initially I did not understand your concern. Fixed the code, so that existing node with changed operands is returned.

Sorry, it took so long to respond!

lib/Target/X86/X86ISelLowering.cpp
34652

Sorry I was very unclear (I ended up confusing myself as well) what I meant was that everything should be the same value type already and C1 was already a node at In.getOperand(1), so you should be able to do something like:

EVT InVT = In.getValueType();
return DAG.getNode(ISD::SMAX, DL, InVT, SMin, In.getOperand(1));

It might even be worth pulling out the InVT to the top of the function so you can use it in the assert as well.

Simon, thanks! I agree that operands have same type. Do you think that it is better to create new node? Or the latest uploaded version where I update the operand of smax is acceptable?

DAG.UpdateNodeOperands(In.getNode(), SMin, In.getOperand(1));
return In;

Simon, thanks! I agree that operands have same type. Do you think that it is better to create new node? Or the latest uploaded version where I update the operand of smax is acceptable?

DAG.UpdateNodeOperands(In.getNode(), SMin, In.getOperand(1));
return In;

A new node is clearer and avoids issues if In is used multiple times.

Fixed according to comment.

RKSimon accepted this revision.May 15 2018, 3:04 AM

LGTM with one minor fix in the assertion

lib/Target/X86/X86ISelLowering.cpp
34623

You should be asserting on scalar bits sizes:

assert(InVT.getScalarSizeInBits() > VT.getScalarSizeInBits() &&
         "Unexpected types for truncate operation");
This revision is now accepted and ready to land.May 15 2018, 3:04 AM
This revision was automatically updated to reflect the committed changes.