[PATCH] R600/SI: Fix V_FRACT hw bug on SI

[Matt Arsenault]

On 02/26/2015 03:42 PM, Marek Olšák wrote:
> +  case AMDGPUIntrinsic::AMDGPU_fract:
> +  case AMDGPUIntrinsic::AMDIL_fraction: // Legacy name.
> +    if (Subtarget->getGeneration() == AMDGPUSubtarget::SOUTHERN_ISLANDS) {
> +      SDValue Op1 = Op.getOperand(1);
> +      SDValue UpperBound = DAG.getConstantFP(BitsToFloat(0x3f7fffff), VT);
> +
> +      // Hardware bug: V_FRACT returns 1.0 for a very small negative input.
> +      // Workaround:
> +      //     isnan(Op1) ? Op1        : min(fract(Op1), 0x3f7fffff) =
> +      // then simplifying...
> +      //     isnan(Op1) ? fract(Op1) : min(fract(Op1), 0x3f7fffff) =
> +      //     0x3f7fffff < fract(Op1) ? 0x3f7fffff : fract(Op1) =
> +      //     fmin_legacy(0x3f7fffff, fract(Op1))
> +      SDValue Frc = DAG.getNode(AMDGPUISD::FRACT, DL, VT, Op1);
> +      return DAG.getNode(AMDGPUISD::FMIN_LEGACY, DL, VT, UpperBound, Frc);
> +    } else
> +      return DAG.getNode(AMDGPUISD::FRACT, DL, VT, Op.getOperand(1));
I don't think this should be implemented in the lowering of the 
intrinsic. I believe this would be better handled by an instruction 
pattern (which for this it looks like it's possible, but maybe not. 
Alternatively in AMDGPUISelDAGToDAG). The first reason is other custom 
lowerings in the future may want to emit the fract node, and those would 
not be handled correctly. The second reason is partially that I think 
this intrinsic should be removed since it's easy to match the x - 
floor(x) pattern and the rest of the compiler understands those 
instructions. There might be special cases that the instruction doesn't 
handle correctly that I'm not aware of which would be a reason to keep it.

Actually, why isn't the workaround to expand it as x - floor(x)? Then 
the intrinsic could be expanded that way, and the workaround would be to 
just not match that pattern on SI.

-Matt