Page MenuHomePhabricator

[X86] Replace (most) X86ISD::SHLD/SHRD usage with ISD::FSHL/FSHR generic opcodes (PR39467)
ClosedPublic

Authored by RKSimon on Fri, Mar 6, 7:50 AM.

Details

Summary

For i32 and i64 cases, X86ISD::SHLD/SHRD are close enough to ISD::FSHL/FSHR that we can use them directly, we just need to account for the operand commutation for SHRD.

The i16 SHLD/SHRD case is annoying as the shift amount is modulo-32 (vs funnel shift modulo-16), so I've added X86ISD::FSHL/FSHR equivalents, which matches the generic implementation in all other terms.

Something I'm slightly concerned with is that ISD::FSHL/FSHR legality is controlled by the Subtarget.isSHLDSlow() feature flag - we don't normally use non-ISA features for this but it allows the DAG combines to continue to operate after legalization in a lot more cases.

The X86 clear_highbits.ll changes are all affected by the same issue - we now have a "FSHR(-1,-1,amt) -> ROTR(-1,amt) -> (-1)" simplification that reduces the dependencies enough for the branch fall through code to mess up. I'm not sure how much of a patch-specific problem this is - tbh if it wasn't for the extra stack usage I wouldn't care much at all. Thoughts?

Diff Detail

Event Timeline

RKSimon created this revision.Fri, Mar 6, 7:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Mar 6, 7:50 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
RKSimon edited the summary of this revision. (Show Details)Fri, Mar 6, 7:54 AM
RKSimon marked an inline comment as done.Fri, Mar 6, 8:03 AM
RKSimon added inline comments.
llvm/test/CodeGen/X86/clear-highbits.ll
3–6

@lebedev.ri Apart from the X86-FALLBACK0 case, can't we enable +cmov on these x86 targets? I can't think of a target that would support any BMI/TBM level without CMOV support.

lebedev.ri added inline comments.Fri, Mar 6, 8:20 AM
llvm/test/CodeGen/X86/clear-highbits.ll
3–6

I don't see why not.

(i think this is the wrong patch)

lebedev.ri requested changes to this revision.Mon, Mar 9, 6:29 AM

Are you sure this is the right patch?

This revision now requires changes to proceed.Mon, Mar 9, 6:29 AM

Sorry, git was being git - will fix in a sec

RKSimon updated this revision to Diff 249099.Mon, Mar 9, 7:44 AM
RKSimon edited the summary of this revision. (Show Details)

regenerate diff

Any more comments? I don't want to rush but I'm keen to get this in as it blocks a number of other patches.

craig.topper added inline comments.Tue, Mar 10, 10:58 AM
llvm/lib/Target/X86/X86ISelLowering.h
37

What is the 'x' after the W?

lebedev.ri accepted this revision.Tue, Mar 10, 11:04 AM

This doesn't look unreasonable to me but best for someone one too look this over, too.

This revision is now accepted and ready to land.Tue, Mar 10, 11:04 AM
RKSimon marked an inline comment as done.Tue, Mar 10, 3:23 PM
RKSimon added inline comments.
llvm/lib/Target/X86/X86ISelLowering.h
37

The instruction suffix for the rr/rm/mr cases - I'll remove it.

RKSimon marked an inline comment as done and an inline comment as not done.Tue, Mar 10, 3:49 PM
RKSimon added inline comments.
llvm/lib/Target/X86/X86ISelLowering.h
37

@craig.topper Other than this are you ok with the patch?

craig.topper accepted this revision.Tue, Mar 10, 4:10 PM

LGTM

llvm/lib/Target/X86/X86ISelLowering.h
37

Yeah

This revision was automatically updated to reflect the committed changes.
RKSimon marked an inline comment as not done.