[PATCH] D129268: Legalise patchpoint arguments.

[Edd Barrett via Phabricator via llvm-commits]

vext01 added a comment.

I'm still unsure what you mean by "No return after else".



================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:5526-5527
+
+  if (Op->getOpcode() == ISD::Constant) {
+    ConstantSDNode *CN = cast<ConstantSDNode>(Op);
+    EVT Ty = Op.getValueType();
----------------
arsenm wrote:
> vext01 wrote:
> > arsenm wrote:
> > > dyn_cast to ConstantSDNode
> > Forgive me. I'm still fairly new to C++, am I doing this right?
> > 
> > ```diff
> > diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
> > index 4ee4ece7d8ef..89c0738f24b0 100644
> > --- a/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
> > +++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
> > @@ -5547,8 +5547,7 @@ SDValue DAGTypeLegalizer::ExpandIntOp_STACKMAP(SDNode *N, unsigned OpNo) {
> >    for (unsigned I = 0; I < OpNo; I++)
> >      NewOps.push_back(N->getOperand(I));
> >  
> > -  if (Op->getOpcode() == ISD::Constant) {
> > -    ConstantSDNode *CN = cast<ConstantSDNode>(Op);
> > +  if (ConstantSDNode *CN = dyn_cast<ConstantSDNode>(Op)) {
> >      EVT Ty = Op.getValueType();
> >      if (CN->getConstantIntValue()->getValue().getActiveBits() < 64) {
> >        NewOps.push_back(
> > @@ -5587,8 +5586,7 @@ SDValue DAGTypeLegalizer::ExpandIntOp_PATCHPOINT(SDNode *N, unsigned OpNo) {
> >    for (unsigned I = 0; I < OpNo; I++)
> >      NewOps.push_back(N->getOperand(I));
> >  
> > -  if (Op->getOpcode() == ISD::Constant) {
> > -    ConstantSDNode *CN = cast<ConstantSDNode>(Op);
> > +  if (ConstantSDNode *CN = dyn_cast<ConstantSDNode>(Op)) {
> >      EVT Ty = Op.getValueType();
> >      if (CN->getConstantIntValue()->getValue().getActiveBits() < 64) {
> >        NewOps.push_back(
> > diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
> > index 25105ee2dbde..0dae6a06ef72 100644
> > --- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
> > +++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
> > @@ -2201,12 +2201,11 @@ void SelectionDAGISel::pushStackMapLiveVariable(SmallVectorImpl<SDValue> &Ops,
> >    // nodes at DAG-construction time.
> >    assert(OpNode->getOpcode() != ISD::FrameIndex);
> >  
> > -  if (OpNode->getOpcode() == ISD::Constant) {
> > +  if (ConstantSDNode *CN = dyn_cast<ConstantSDNode>(OpNode)) {
> >      Ops.push_back(
> >          CurDAG->getTargetConstant(StackMaps::ConstantOp, DL, MVT::i64));
> >      Ops.push_back(
> > -        CurDAG->getTargetConstant(cast<ConstantSDNode>(OpNode)->getZExtValue(),
> > -                                  DL, OpVal.getValueType()));
> > +        CurDAG->getTargetConstant(CN->getZExtValue(), DL, OpVal.getValueType()));
> >    } else {
> >      Ops.push_back(OpVal);
> >    }
> > ```
> > 
> > This makes tests fail, so can't be equivalent.
> The slight difference is ConstantSDNode accepts Constant and TargetConstant but I would expect consistency at this point
The first two hunks of the above diff are fine, it's the last one that causes test failures.

Not sure why, but I've omitted that change for now anyway.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129268/new/

https://reviews.llvm.org/D129268