[ONNX] Fix Onnx.Hardsigmoid lowering #3239
Conversation
Signed-Off By: Vivek Khandelwal <vivekkhandelwal1424@gmail.com>
| rewriter.getF64FloatAttr(1.0)); | ||
| Value alphaMulXPlusBeta = rewriter.create<Torch::AtenAddScalarOp>( | ||
| binder.getLoc(), resultType, alphaMulX, constBeta, | ||
| /*alpha=*/constOne); |
There was a problem hiding this comment.
AtenAddScalarOp has the input signature (tensor, float, float) and performs result = operand[0] + operand[1]*operand[2], correct? I feel like this op would be way more useful if it multiplied the tensor operand[0] with operand[2].
There was a problem hiding this comment.
i wonder if this is just to be consistent with the aten.add.Tensor op
where you get tensor1+tensor2*alpha
There was a problem hiding this comment.
but also this leave the question of why we have the alpha in the first place ... for quantization maybe?
| /*alpha=*/constAlpha); | ||
| Value alphaMulX = rewriter.create<Torch::AtenMulScalarOp>( | ||
| binder.getLoc(), resultType, tensorOperand, constAlpha); | ||
| Value constOne = rewriter.create<Torch::ConstantFloatOp>( |
There was a problem hiding this comment.
There is an aten minimum op that compares the result of the linear expression with a constant oneTensor a few lines below this, and for some reason it is using a constant int one instead of a constant float one. I'm sure it will get cast to a float at some point, but maybe this could just be cleaned up here to use your constOne.
If you make a change to use constOne for the oneTensor, it might also make sense just to change constantZero below to a float as well.
| rewriter.getF64FloatAttr(1.0)); | ||
| Value alphaMulXPlusBeta = rewriter.create<Torch::AtenAddScalarOp>( | ||
| binder.getLoc(), resultType, alphaMulX, constBeta, | ||
| /*alpha=*/constOne); |
There was a problem hiding this comment.
but also this leave the question of why we have the alpha in the first place ... for quantization maybe?
Signed-Off By: Vivek Khandelwal vivekkhandelwal1424@gmail.com