This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Fix DAG selection for cmps for fp16 type
ClosedPublic

Authored by weimingz on May 4 2016, 8:34 AM.

Details

Summary

When emitting comparison for fp16, in addition to promote the LHS and RHS to fp32, we need to change the VT as well.

Diff Detail

Repository
rL LLVM

Event Timeline

weimingz updated this revision to Diff 56157.May 4 2016, 8:34 AM
weimingz retitled this revision from to [AArch64] Fix DAG selection for cmps for fp16 type.
weimingz updated this object.
weimingz set the repository for this revision to rL LLVM.
weimingz added a subscriber: llvm-commits.

I think the real bug here is in emitComparison, which has exactly this code to promote the arguments but still produces an f16 result. Changing that to f32 ought to fix things (though the dataflow between FCMP and FCCMP is a bit dubious, it's playing rather loose with types given that NZCV is the only register actually modified).

test/CodeGen/AArch64/half.ll
87–88

There are more ways this could go wrong than simply not emitting an fcmp/fccmp. I think you should check for an fcvt producing an s register that then gets used in the comparisons.

weimingz updated this revision to Diff 56845.May 10 2016, 6:07 PM
weimingz updated this object.
weimingz removed rL LLVM as the repository for this revision.

This fixes the crash of "llc: tip/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:6340: void llvm::SelectionDAG::ReplaceAllUsesWith(llvm::SDNode *, llvm::SDNode *): Assertion `(!From->hasAnyUseOfValue(i) || From->getValueType(i) == To->getValueType(i)) && "Cannot use this version of ReplaceAllUsesWith!"' failed.

t.p.northover accepted this revision.May 10 2016, 6:18 PM
t.p.northover added a reviewer: t.p.northover.

Thanks Weiming. Looks good to me.

Tim.

This revision is now accepted and ready to land.May 10 2016, 6:18 PM
This revision was automatically updated to reflect the committed changes.

Thank you, Tim! And do we need to guard the promotion of fp16->fp32 with !HasFullFP16() check? If some subtarget has full fp16 support, the promotion is not needed, is it?

lib/Target/AArch64/AArch64ISelLowering.cpp
1412

Operands get from a chained SETCC

mgrang added a subscriber: mgrang.May 10 2016, 7:39 PM
mgrang added inline comments.
llvm/trunk/test/CodeGen/AArch64/half.ll
86 ↗(On Diff #56847)

It would be good to follow the convention of having a space between ; and CHECK (see other instances in this file):