Page MenuHomePhabricator

[DAG][X86] Convert isNegatibleForFree/GetNegatedExpression to a target hook (PR42863)
ClosedPublic

Authored by RKSimon on Sep 13 2019, 8:39 AM.

Details

Summary

This patch converts the DAGCombine isNegatibleForFree/GetNegatedExpression into overridable TLI hooks and includes a demonstration X86 implementation.

The intention is to let us extend existing FNEG combines to work more generally with negatible float ops, allowing it work with target specific combines and opcodes (e.g. X86's FMA variants).

Unlike the SimplifyDemandedBits, we can't just handle target nodes through a Target callback, we need to do this as an override to allow targets to handle generic opcodes as well. This does mean that the target implementations has to duplicate some checks (recursion depth etc.).

I've only begun to replace X86's FNEG handling here, handling FMADDSUB/FMSUBADD negation and some low impact codegen changes (some FMA negatation propagation). We can build on this in future patches.

@arsenm I haven't touched AMDGPUTargetLowering::performFNegCombine but the idea would be that this is refactored as well. I don't think I've missed anything you'd need, but if you wish I can attempt an initial implementation.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon created this revision.Sep 13 2019, 8:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2019, 8:39 AM
Herald added subscribers: tpr, wdng. · View Herald Transcript
spatel added inline comments.Wed, Sep 18, 5:43 AM
lib/CodeGen/SelectionDAG/TargetLowering.cpp
5333–5334 ↗(On Diff #220112)

It would be better to fix this first as an NFC (give the '6' a name) since it's a repeated/referenced magic number in the existing code.

lib/Target/X86/X86ISelLowering.cpp
41928 ↗(On Diff #220112)

it has -> they have

41947 ↗(On Diff #220112)

Curious about that subtarget check - is there a regression test where we have an FMA opcode on a non-FMA CPU?

43021–43022 ↗(On Diff #220112)

I didn't grok this on 1st read, so it's worth a code comment like:
// Copy the multiplied ops and replace the addend.

or just do the usual local naming/usage:

SDValue N0 = N->getOperand(0);
SDValue N1 = N->getOperand(1);
SDValue N2 = N->getOperand(2);

Cheers, I'll deal with standardizing the recursion depth constant in a sec

lib/Target/X86/X86ISelLowering.cpp
41947 ↗(On Diff #220112)

Its there to prevent the X86ISD FMA opcode being generated on non-FMA targets. We fallback on the generic FMA handling which can still do some useful combines.

43021–43022 ↗(On Diff #220112)

OK - I was trying to merge the N->getNumOperands() == 4 code path for the FMADDSUB_RND cases - but I understand its not as easy to understand at a glance.

spatel accepted this revision.Thu, Sep 19, 7:14 AM

LGTM

lib/Target/X86/X86ISelLowering.cpp
41928 ↗(On Diff #220112)

grammar nit still here :)

This revision is now accepted and ready to land.Thu, Sep 19, 7:14 AM
This revision was automatically updated to reflect the committed changes.

This was reverted due to a compile time regression at rL372756