This is an archive of the discontinued LLVM Phabricator instance.

[Codegen] Alter the default promotion for add_sat and sub_sat
ClosedPublic

Authored by dmgreen on Oct 8 2019, 7:19 AM.

Details

Summary

The default promotion for the add_sat/sub_sat nodes currently does:

//   1. ANY_EXTEND iN to iM
//   2. SHL by M-N
//   3. [US][ADD|SUB]SAT
//   4. L/ASHR by M-N

If the promoted add_sat or sub_sat node is not legal, this can produce code that effectively does a lot of shifting (and requiring large constants to be materialised) just to use the overflow flag. It is simpler to just do the saturation manually, using the higher bitwidth addition and a min/max against the saturating bounds. That is what this patch attempts to do.

A few points of interest:

  • This still uses the existing promotion when the promoted add/sub_sat is legal. In many situations (but not all) it would probably be better to just perform the new promotion.
  • It always creates a MIN and MAX node, even if they are not legal. The alternative would be to be to create a cmp/select pair, which these will legalise into anyway.
  • We only have AArch64 and X86 tests. I have added ARM (for which just the differences are shown here). I'm happy to add other architectures if people are interested.

Diff Detail

Event Timeline

dmgreen created this revision.Oct 8 2019, 7:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2019, 7:19 AM
lebedev.ri retitled this revision from [Codegen] Alter the default promotion for saturating adds and subs to [Codegen] Alter the default promotion for add_sat and sub_sat.Oct 8 2019, 7:49 AM
lebedev.ri added a reviewer: nikic.
nikic accepted this revision.Oct 8 2019, 1:14 PM

LGTM

This still uses the existing promotion when the promoted add/sub_sat is legal. In many situations (but not all) it would probably be better to just perform the new promotion.

I guess at least for the unsigned case, it would be reasonable to use the shift+sat expansion only if both the sat is legal *and* min/max is not legal. The umax+sub expansion at least is clearly better than usubsat + 3 shifts.

But that's probably better left for a followup patch.

This revision is now accepted and ready to land.Oct 8 2019, 1:14 PM

Thanks. I'll put a patch together showing the differences. It was only really the odd types like "i4" that changed IIRC. Some of them were looking better in places, worse in others. Making it dependent on whether min/max are available sounds like a good idea.

This revision was automatically updated to reflect the committed changes.