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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
---|---|---|
12714 | 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. |
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 ? |
LGTM - cheers
llvm/include/llvm/CodeGen/TargetLowering.h | ||
---|---|---|
3547 | No good ideas - hence my vague "its annoying" comment :-) |
llvm/include/llvm/CodeGen/TargetLowering.h | ||
---|---|---|
3547 | Thank you for the review :) |
Its annoying that we're introducing negateExpression that will then go away again in D77319 - is there anyway to avoid creating negateExpression?