This is an archive of the discontinued LLVM Phabricator instance.

Differential D34579

Fold fneg and fabs like multiplications
ClosedPublic

Authored by rampitec on Jun 23 2017, 4:06 PM.

Details

Reviewers
arsenm
vpykhtin
majnemer
Commits
rGa45584bebebc: Fold fneg and fabs like multiplications
rL306592: Fold fneg and fabs like multiplications
Summary

Given no NaNs and no signed zeroes it folds:

(fmul X, (select (fcmp X > 0.0), -1.0, 1.0)) -> (fneg (fabs X))
(fmul X, (select (fcmp X > 0.0), 1.0, -1.0)) -> (fabs X)

Diff Detail

Event Timeline

rampitec created this revision.Jun 23 2017, 4:06 PM
Herald added subscribers: nhaehnle, wdng. · View Herald TranscriptJun 23 2017, 4:06 PM
hfinkel added a subscriber: hfinkel.Jun 23 2017, 4:18 PM
hfinkel added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9772

According to the docs in include/llvm/CodeGen/ISDOpcodes.h, for floating point you should handle the SETLT and friends as well (these just mean that it is undefined if the input is NaN).

9776

Visually, I'd prefer that the std::swap be on its own line below this one. Otherwise, it looks like all of the condition code fall through to the bottom.

rampitec updated this revision to Diff 103801.Jun 23 2017, 4:35 PM
rampitec marked 2 inline comments as done.

Addressed review comments.

apinski-cavium added a subscriber: apinski-cavium.Jun 23 2017, 4:39 PM

(fmul X, (select (fcmp X > 0.0), -1.0, 1.0)) -> (fneg X)

Is not correct.

It should be:
(fmul X, (select (fcmp X > 0.0), -1.0, 1.0)) -> (fneg (fabs X))

If X was -2.0, (fmul X, (select (fcmp X > 0.0), -1.0, 1.0)) is (fmul -2.0, (select false, -1.0, 1.0) ) =>(fmul -2.0, 1.0 ) => -2.0. Which is different from (fneg -2.0) => 2.0.

rampitec planned changes to this revision.Jun 23 2017, 4:44 PM
In D34579#789534, @apinski-cavium wrote:

(fmul X, (select (fcmp X > 0.0), -1.0, 1.0)) -> (fneg X)

Is not correct.

It should be:
(fmul X, (select (fcmp X > 0.0), -1.0, 1.0)) -> (fneg (fabs X))

If X was -2.0, (fmul X, (select (fcmp X > 0.0), -1.0, 1.0)) is (fmul -2.0, (select false, -1.0, 1.0) ) =>(fmul -2.0, 1.0 ) => -2.0. Which is different from (fneg -2.0) => 2.0.

Ugh... right.

rampitec updated this revision to Diff 103805.Jun 23 2017, 4:54 PM
rampitec edited the summary of this revision. (Show Details)

Fixed (fneg (fabs X)) case.

arsenm added inline comments.Jun 23 2017, 5:07 PM
test/CodeGen/AMDGPU/fold-fmul-to-neg-abs.ll
34

Needs test for fneg/fabs case

rampitec added inline comments.Jun 23 2017, 5:10 PM
test/CodeGen/AMDGPU/fold-fmul-to-neg-abs.ll
34

That is the first test. fneg(fabs) became v_or_b32. Initially it was an incorrect xor.

apinski-cavium removed a subscriber: apinski-cavium.Jun 23 2017, 5:11 PM
vpykhtin accepted this revision.Jun 27 2017, 3:12 AM

LGTM.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
9767

I understand what you meant but at first glance it looks like "if ( 2 * 2 == 4 )" :-) Should it be named like TrueOpnd and FalseOpnd?

This revision is now accepted and ready to land.Jun 27 2017, 3:12 AM
rampitec updated this revision to Diff 104208.Jun 27 2017, 10:33 AM
rampitec marked an inline comment as done.

Renamed variables True and False into TrueOpnd and FalseOpnd as suggested.

Closed by commit rL306592: Fold fneg and fabs like multiplications (authored by rampitec). · Explain WhyJun 28 2017, 1:26 PM
This revision was automatically updated to reflect the committed changes.
apinski-cavium added a comment.Jun 28 2017, 1:26 PM

Note why don't you
fold "(select (fcmp X > 0.0), -1.0, 1.0)" into copysign(1.0, X)
fold "(select (fcmp X > 0.0), 1.0, -1.0)" into copysign(1.0, -X)
?

I did that for GCC and then optimized:
X * copysign(1.0, X) into abs(X)
X * copysign(1.0, -X) into -abs(X)

rampitec added a subscriber: apinski-cavium.Jun 28 2017, 1:31 PM
In D34579#794303, @apinski-cavium wrote:

Note why don't you
fold "(select (fcmp X > 0.0), -1.0, 1.0)" into copysign(1.0, X)
fold "(select (fcmp X > 0.0), 1.0, -1.0)" into copysign(1.0, -X)
?

I did that for GCC and then optimized:
X * copysign(1.0, X) into abs(X)
X * copysign(1.0, -X) into -abs(X)

Hm.. That can be an interesting idea too.

Revision Contents

PathSize
lib/
CodeGen/
SelectionDAG/
46 lines
test/
CodeGen/
AMDGPU/
37 lines

Diff 103805

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 9,745 Lines • ▼ Show 20 Lines if (char RHSNeg = isNegatibleForFree(N1, LegalOperations, TLI, &Options)) {
if (LHSNeg == 2 || RHSNeg == 2) if (LHSNeg == 2 || RHSNeg == 2)
return DAG.getNode(ISD::FMUL, DL, VT, return DAG.getNode(ISD::FMUL, DL, VT,
GetNegatedExpression(N0, DAG, LegalOperations), GetNegatedExpression(N0, DAG, LegalOperations),
GetNegatedExpression(N1, DAG, LegalOperations), GetNegatedExpression(N1, DAG, LegalOperations),
Flags); Flags);
} }
} }
// fold (fmul X, (select (fcmp X > 0.0), -1.0, 1.0)) -> (fneg (fabs X))
// fold (fmul X, (select (fcmp X > 0.0), 1.0, -1.0)) -> (fabs X)
if (Flags.hasNoNaNs() && Flags.hasNoSignedZeros() &&
(N0.getOpcode() == ISD::SELECT || N1.getOpcode() == ISD::SELECT) &&
TLI.isOperationLegal(ISD::FABS, VT)) {
SDValue Select = N0, X = N1;
if (Select.getOpcode() != ISD::SELECT)
std::swap(Select, X);
SDValue Cond = Select.getOperand(0);
auto True = dyn_cast<ConstantFPSDNode>(Select.getOperand(1));
auto False = dyn_cast<ConstantFPSDNode>(Select.getOperand(2));
if (True && False &&
vpykhtinUnsubmitted
Done
ReplyInline Actions

I understand what you meant but at first glance it looks like "if ( 2 * 2 == 4 )" :-) Should it be named like TrueOpnd and FalseOpnd?

vpykhtin: I understand what you meant but at first glance it looks like "if ( 2 * 2 == 4 )" :-) Should it…
Cond.getOpcode() == ISD::SETCC && Cond.getOperand(0) == X &&
isa<ConstantFPSDNode>(Cond.getOperand(1)) &&
cast<ConstantFPSDNode>(Cond.getOperand(1))->isExactlyValue(0.0)) {
ISD::CondCode CC = cast<CondCodeSDNode>(Cond.getOperand(2))->get();
switch (CC) {
hfinkelUnsubmitted
Done
ReplyInline Actions

According to the docs in include/llvm/CodeGen/ISDOpcodes.h, for floating point you should handle the SETLT and friends as well (these just mean that it is undefined if the input is NaN).

hfinkel: According to the docs in include/llvm/CodeGen/ISDOpcodes.h, for floating point you should…
default: break;
case ISD::SETOLT:
case ISD::SETULT:
case ISD::SETOLE:
hfinkelUnsubmitted
Done
ReplyInline Actions

Visually, I'd prefer that the std::swap be on its own line below this one. Otherwise, it looks like all of the condition code fall through to the bottom.

hfinkel: Visually, I'd prefer that the std::swap be on its own line below this one. Otherwise, it looks…
case ISD::SETULE:
case ISD::SETLT:
case ISD::SETLE:
std::swap(True, False);
// Fall through
case ISD::SETOGT:
case ISD::SETUGT:
case ISD::SETOGE:
case ISD::SETUGE:
case ISD::SETGT:
case ISD::SETGE:
if (True->isExactlyValue(-1.0) && False->isExactlyValue(1.0) &&
TLI.isOperationLegal(ISD::FNEG, VT))
return DAG.getNode(ISD::FNEG, DL, VT,
DAG.getNode(ISD::FABS, DL, VT, X));
if (True->isExactlyValue(1.0) && False->isExactlyValue(-1.0))
return DAG.getNode(ISD::FABS, DL, VT, X);
break;
}
}
}
// FMUL -> FMA combines: // FMUL -> FMA combines:
if (SDValue Fused = visitFMULForFMADistributiveCombine(N)) { if (SDValue Fused = visitFMULForFMADistributiveCombine(N)) {
AddToWorklist(Fused.getNode()); AddToWorklist(Fused.getNode());
return Fused; return Fused;
} }
return SDValue(); return SDValue();
} }
▲ Show 20 Lines • Show All 7,080 Lines • Show Last 20 Lines

test/CodeGen/AMDGPU/fold-fmul-to-neg-abs.ll

  • This file was added.
; RUN: llc -march=amdgcn -verify-machineinstrs < %s | FileCheck -check-prefix=GCN %s
; GCN-LABEL: {{^}}fold_mul_neg:
; GCN: load_dword [[V:v[0-9]+]]
; GCN: v_or_b32_e32 [[NEG:v[0-9]]], 0x80000000, [[V]]
; GCN: store_dword [[NEG]]
define amdgpu_kernel void @fold_mul_neg(float addrspace(1)* %arg) {
%tid = tail call i32 @llvm.amdgcn.workitem.id.x()
%gep = getelementptr inbounds float, float addrspace(1)* %arg, i32 %tid
%v = load float, float addrspace(1)* %gep, align 4
%cmp = fcmp fast ogt float %v, 0.000000e+00
%sel = select i1 %cmp, float -1.000000e+00, float 1.000000e+00
%mul = fmul fast float %v, %sel
store float %mul, float addrspace(1)* %gep, align 4
ret void
}
; GCN-LABEL: {{^}}fold_mul_abs:
; GCN: load_dword [[V:v[0-9]+]]
; GCN: v_and_b32_e32 [[ABS:v[0-9]]], 0x7fffffff, [[V]]
; GCN: store_dword [[ABS]]
define amdgpu_kernel void @fold_mul_abs(float addrspace(1)* %arg) {
%tid = tail call i32 @llvm.amdgcn.workitem.id.x()
%gep = getelementptr inbounds float, float addrspace(1)* %arg, i32 %tid
%v = load float, float addrspace(1)* %gep, align 4
%cmp = fcmp fast olt float %v, 0.000000e+00
%sel = select i1 %cmp, float -1.000000e+00, float 1.000000e+00
%mul = fmul fast float %v, %sel
store float %mul, float addrspace(1)* %gep, align 4
ret void
}
arsenmUnsubmitted
Not Done
ReplyInline Actions

Needs test for fneg/fabs case

arsenm: Needs test for fneg/fabs case
rampitecAuthorUnsubmitted
Not Done
ReplyInline Actions

That is the first test. fneg(fabs) became v_or_b32. Initially it was an incorrect xor.

rampitec: That is the first test. fneg(fabs) became v_or_b32. Initially it was an incorrect xor.
declare i32 @llvm.amdgcn.workitem.id.x() #0
attributes #0 = { nounwind readnone speculatable }