This is an archive of the discontinued LLVM Phabricator instance.

[DAG] Add a generic expansion for SHIFT_PARTS opcodes using funnel shifts
ClosedPublic

Authored by RKSimon on May 6 2021, 4:05 AM.

Details

Summary

Based off a discussion on D89281 - where the aarch64 implementations were being replaced to use funnel shifts.

Any target that has efficient funnel shift lowering can handle the shift parts expansion using the same expansion, avoiding a lot of duplication.

I've generalized the x86 implementation and moved it to TargetLowering - so far I've found that aarch64 and amdgpu benefit, but many other targets (ARM, PowerPC + RISCV in particular) could easily use this with a few minor improvements to their funnel shift lowering (or the folding of their target ops that funnel shifts lower to).

NOTE: I'm trying to avoid adding full SHIFT_PARTS legalizer handling as I think it might actually be possible to remove these opcodes in the medium-term and use funnel shift / libcall expansion directly.

Diff Detail

Event Timeline

RKSimon created this revision.May 6 2021, 4:05 AM
RKSimon requested review of this revision.May 6 2021, 4:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2021, 4:05 AM
foad added a comment.May 6 2021, 4:22 AM

The new TargetLowering::expandShiftParts LGTM. I haven't really looked at the rest of the patch. (For the record, although I'm interested in the AMDGPU target in general, I'm not interested in the R600-based subtargets.)

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
6625–6626

Nit: it's not just that we "can't rely on the results of FSHL/FSHR"; both halves of the result have to be calculated differently for large shift amounts.

RKSimon updated this revision to Diff 343361.May 6 2021, 4:56 AM

clean up comment

Matt added a subscriber: Matt.May 6 2021, 7:29 AM

LGTM, but someone familiar with AMDGPU should look at those changes.

LGTM, but someone familiar with AMDGPU should look at those changes.

@arsenm @foad Would you be willing to accept this?

foad accepted this revision.May 7 2021, 1:51 AM

LGTM, but someone familiar with AMDGPU should look at those changes.

@arsenm @foad Would you be willing to accept this?

I don't see anything that's obviously worse, and some cases like shift of an i64 constant by variable look significantly better, so OK.

This revision is now accepted and ready to land.May 7 2021, 1:51 AM
This revision was landed with ongoing or failed builds.May 7 2021, 5:12 AM
This revision was automatically updated to reflect the committed changes.