This is an archive of the discontinued LLVM Phabricator instance.

[NFC][DAGCombine] Adding three helper functions and change the getNegatedExpression to negateExpression
ClosedPublic

Authored by steven.zhang on Apr 16 2020, 4:45 AM.

Details

Summary

This is a NFC patch for D77319. The idea is to hide the getNegatibleCost inside the getNegatedExpression() to have it return null if the cost is expensive, and add some helper function for easy to use. And rename the old getNegatedExpression to negateExpression to avoid the semantic conflict. In the latter patch, we will remove the getNegatibleCost and negateExpression.

Diff Detail

Event Timeline

steven.zhang created this revision.Apr 16 2020, 4:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2020, 4:45 AM
steven.zhang marked an inline comment as done.Apr 16 2020, 4:53 AM
steven.zhang added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12693

This lines of code is a bit tricky, we can write it as

TargetLowering::NegatibleCost CostN0 =
    TargetLowering::NegatibleCost::Expensive;
TargetLowering::NegatibleCost CostN1 =
    TargetLowering::NegatibleCost::Expensive;
SDValue NegN0 =
    TLI.getNegatedExpression(N0, DAG, LegalOperations, ForCodeSize, CostN0);
SDValue NegN1 =
    TLI.getNegatedExpression(N1, DAG, LegalOperations, ForCodeSize, CostN1);
if (NegN0 && NegN1 &&
    (CostN0 == TargetLowering::NegatibleCost::Cheaper ||
     CostN1 == TargetLowering::NegatibleCost::Cheaper)) {
  return DAG.getNode(ISD::FMUL, DL, VT, NegN0, NegN1, Flags);
}

However, they are not semantics the same, as current implementation don't want to negate the expression if the cost of negate N0 and N1 are both neutral while the above implementation did. Technical speaking, it won't have bad impact as we don't change the FMUL if both are neutral. However, it changes the order of the node to combine, which change the PowerPC test llvm/test/CodeGen/PowerPC/qpx-recipest.ll we see in D77319 Maybe, we need another patch to change the semantics and PowerPC test.

RKSimon added inline comments.Apr 17 2020, 6:47 AM
llvm/include/llvm/CodeGen/TargetLowering.h
3547

Its annoying that we're introducing negateExpression that will then go away again in D77319 - is there anyway to avoid creating negateExpression?

steven.zhang marked an inline comment as done.Apr 17 2020, 6:49 PM
steven.zhang added inline comments.
llvm/include/llvm/CodeGen/TargetLowering.h
3547

Old getNegatedExpression means always negate the expr and never fail. We are changing the sematics to allow it failing if return null. And we need both the old getNegatedExpression and new one in this nfc patch. So I have to rename it. The negateExpression is only called inside target lowering,and merged with getNegatibleCost into getNegatedExpression in D77319. I don't have good idea not to rename it. Do you have any idea ?

Gentle ping ...

RKSimon accepted this revision.Apr 24 2020, 10:30 AM

LGTM - cheers

llvm/include/llvm/CodeGen/TargetLowering.h
3547

No good ideas - hence my vague "its annoying" comment :-)

This revision is now accepted and ready to land.Apr 24 2020, 10:30 AM
steven.zhang marked an inline comment as done.Apr 26 2020, 7:01 PM
steven.zhang added inline comments.
llvm/include/llvm/CodeGen/TargetLowering.h
3547

Thank you for the review :)

steven.zhang marked an inline comment as done.Apr 26 2020, 9:06 PM
steven.zhang added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12693

I suddenly realize that, the change here is still not strict-NFC, because we change the DAG when trying to get the cheaper negated expression of N0, which will affect the N1. I will leave this part unchanged and do it in D77319.

This revision was automatically updated to reflect the committed changes.
steven.zhang marked an inline comment as done.Apr 26 2020, 9:16 PM
steven.zhang added inline comments.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12693

Do it in D78347 I mean.