[PATCH] D81537: [PowerPC] Support constrained fp operation for scalar fptosi/fptoui
Ulrich Weigand via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 4 05:50:04 PDT 2020
uweigand added a comment.
This doesn't look correct. As far as I can see, none of the conversion functions were actually changed to handle strict operations. For one, you'll need strict variants of all the PowerPC-specific conversion operations, use them in all the conversion subroutines, and consistently track their chain nodes.
The patch only adds a strict variant of the direct move, which seems to me the only operation where actually a strict version is **not** required ...
================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8230
+ Src = DAG.getNode(ISD::STRICT_FP_EXTEND, dl, {MVT::f64, MVT::Other},
+ {Op.getOperand(0), Src});
+ else
----------------
This doesn't look right. The Chain produced by this strict node just vanishes, this cannot be correct.
================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8247
Conv = DAG.getNode(IsSigned ? PPCISD::FCTIDZ : PPCISD::FCTIDUZ, dl,
MVT::f64, Src);
}
----------------
And you need strict versions of all these conversion operations, I'd assume.
================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8258
+ Op.getOpcode() == ISD::STRICT_FP_TO_SINT;
// Convert the FP value to an int value through memory.
----------------
Nothing in the function actually handles strict nodes, that cannot be right.
================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8307
+ return DAG.getNode(PPCISD::MFVSR, dl, Op.getValueType(),
+ convertFPToInt(Op, DAG, Subtarget));
}
----------------
Why do we need a strict version of a plain move?
================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8312
const SDLoc &dl) const {
- SDValue Src = Op.getOperand(0);
+ SDValue Src = Op.getOperand(Op->isStrictFPOpcode() ? 1 : 0);
// FP to INT conversions are legal for f128.
----------------
Again, nothing in this function actually handles strict nodes ...
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81537/new/
https://reviews.llvm.org/D81537
More information about the llvm-commits
mailing list