Lowering floating point division for 32-bit using IEEE 754

[wdng]

Summary

Lowering floating point division for 32-bit using IEEE 754

[arsenm]

lib/Target/AMDGPU/SIISelLowering.cpp

42

Description is inaccurate. Should say something about faster 2.5 ulp fdiv

2031

No space after (

2032

Break on same line

2034

Indentation looks wrong

2047

Don't all caps ON

2076

You don't need the else

[arsenm]

Needs tests

[arsenm]

You should add more tests that use the fast math flags, and also run with unsafe-fp-math enabled on the function

test/CodeGen/AMDGPU/fdiv.ll

4 ↗

(On Diff #58637)

This should come before the r600 line

81–82 ↗

(On Diff #58637)

-DAG does not work as expected if you specify the same string multiple times in the same -DAG sequence. I would reduce the vector tests to a differentiating instruction or two per element (e.g. div_scale + div_fixup)

[wdng]

Hi Matt, You commented that "You should add more tests that use the fast math flags, and also run with unsafe-fp-math enabled on the function". Looks like fast math is not a llc flag.
Could you explain in more details about using the fast math flags? e.g. What's the fast math flag? How the compiler backend retrieves the fast math flag? what code should we generate for fast-math? Thanks a lot!

[arsenm]

The llc flag is -enable-unsafe-fp-math. There is also the per-function attribute "unsafe-fp-math"="true". We should probably use that, but I discovered a while ago that if one function has it used, the others must have "unsafe-fp-math"="false" because there is a bug where the setting is reset between functions

[wdng]

The new introduced flag " -amdgpu-fast-fdiv" is used to control use old and new added IEEE754 div implementation. For the -enable-unsafe-fp-math, could you please let me know which part of code will be tested? Thanks!

[arsenm]

The test should show which division implementation you get when unsafe math is enabled. There also need to be tests for the fast math flags on the individual instructions

[wdng]

Modify & add LIT tests based on Matt's comments

[arsenm]

You still need to add tests that only use the fast math flags

lib/Target/AMDGPU/SIISelLowering.cpp

2081–2082

Variables should be capitalized and camel case

[wdng]

Any comments about my latest changes? Thanks!

[wdng]

Capitalize variables to follow the camel case.

[arsenm]

In D20557#444512, @wdng wrote:

Any comments about my latest changes? Thanks!

Still need tests with the individual fast math flags

[wdng]

In D20557#444692, @arsenm wrote:

In D20557#444512, @wdng wrote:

Any comments about my latest changes? Thanks!

Still need tests with the individual fast math flags

Not quite clear about the individual fast math? Could you please describe in details?

[arsenm]

In D20557#444720, @wdng wrote:

In D20557#444692, @arsenm wrote:

In D20557#444512, @wdng wrote:

Any comments about my latest changes? Thanks!

Still need tests with the individual fast math flags

Not quite clear about the individual fast math? Could you please describe in details?

There should be tests with just the fast math flags as described here: http://llvm.org/docs/LangRef.html#fast-math-flags

Rather than the globally enabled fast math option.

[wdng]

In D20557#444721, @arsenm wrote:

In D20557#444720, @wdng wrote:

In D20557#444692, @arsenm wrote:

In D20557#444512, @wdng wrote:

Any comments about my latest changes? Thanks!

Still need tests with the individual fast math flags

Not quite clear about the individual fast math? Could you please describe in details?

There should be tests with just the fast math flags as described here: http://llvm.org/docs/LangRef.html#fast-math-flags

Rather than the globally enabled fast math option.

Looks like compiler chooses a different implementation for fpdiv if we have "-enable-unsafe-fp-math" flag enabled when using llc. Generated *.s does show different ISA output, but the breakpoint which I put at SITargetLowering::LowerFDIV is never reached. Is this correct?

[wdng]

Based on Matt's comments: Added tests (Merge Changpeng's LIT tests) with just the fast math flags as described here: http://llvm.org/docs/LangRef.html#fast-math-flags, rather than the globally enabled fast math option.

[arsenm]

test/CodeGen/AMDGPU/fdiv.ll

4 ↗

(On Diff #59137)

fast math + -amdgpu-fast-fdiv would be another combination to try

58 ↗

(On Diff #59137)

There should be one that only uses arcp as well

[wdng]

test/CodeGen/AMDGPU/fdiv.ll

4 ↗

(On Diff #59137)

Do you mean fast-math flag + "-amdgpu-fast-fdiv" ?

[arsenm]

test/CodeGen/AMDGPU/fdiv.ll

4 ↗

(On Diff #59137)

Yes

[wdng]

test/CodeGen/AMDGPU/fdiv.ll

4 ↗

(On Diff #59137)

Actually combining fast math + -amdgpu-fast-fdiv has been covered in these tests. When fast math flags + -enable-unsafe-fp-math are enabled (fast, arcp, etc.), " if (Flags->hasAllowReciprocal()) { ... } " will be executed. Orignal less accurate fpdiv implementation will be executed when only enabling the -amdgpu-fast-fdiv flag. If we enable fast math + -amdgpu-fast-fdiv, less accurate fpdiv will be tested.

[wdng]

Added tests for fast-math arcp.

[arsenm]

LGTM

test/CodeGen/AMDGPU/fdiv.ll

4 ↗

(On Diff #59627)

Is it? This is what I'm unsure about and why I think there should be an additional test. Are the individual math node flags set if these are globally enabled?

[artem.tamazov]

test/CodeGen/AMDGPU/fdiv.ll

50–55 ↗

(On Diff #59627)

AFAIK there should be:

1. Two v_div_scale prior v_rcp
2. Two (not one) v_fma prior v_mul
3. Three (not one) v_fma after v_mul
4. v_div_fmas prior v_div_fixup.

Perhaps the test should check that.

[wdng]

test/CodeGen/AMDGPU/fdiv.ll

50–55 ↗

(On Diff #59627)

Based on Matt's comment: "-DAG does not work as expected if you specify the same string multiple times in the same -DAG sequence". So, we reduce the vector tests to a differentiating instruction or two per element (e.g. div_scale + div_fixup).

[artem.tamazov]

test/CodeGen/AMDGPU/fdiv.ll

50–55 ↗

(On Diff #59627)

OK. However, v_div_fmas_f32 is still missing. Perhaps this is not important.

[wdng]

Fixed LIT test failed for frem.ll and rsq.ll

[arsenm]

test/CodeGen/AMDGPU/frem.ll

1–3 ↗

(On Diff #60081)

This should also test without -amdgpu-fast-fdiv. -mcpu=SI should also be removed

[cfang]

test/CodeGen/AMDGPU/frem.ll

1–3 ↗

(On Diff #60081)

Actually I don't suggest we test with -amdgpu-fast-fdiv here at all. This option has already been tested in fdiv.ll, and testing here does not provide any additional value.

[wdng]

Modified LIT test based on Matt's and Changpeng's suggestion.

[wdng]

test/CodeGen/AMDGPU/fdiv.ll

50–55 ↗

(On Diff #60120)

Yes.

[cfang]

LGTM

Please go ahead to commit!

lib/Target/AMDGPU/SIISelLowering.cpp

 
 using namespace llvm;
 
+// -amdgpu-fast-fdiv - Command line option to enable the IEEE 754 fpdiv computation in the passes.
+static cl::opt<bool> EnableAMDGPUFastFDIV(
+  "amdgpu-fast-fdiv", 
+  cl::desc("Enable IEEE 754 fpdiv computation"), 
+  cl::init(false));
+
 static unsigned findFirstFreeSGPR(CCState &CCInfo) {
   unsigned NumSGPRs = AMDGPU::SGPR_32RegClass.getNumRegs();
   for (unsigned Reg = 0; Reg < NumSGPRs; ++Reg) {

 }
 
 SDValue SITargetLowering::LowerFDIV32(SDValue Op, SelectionDAG &DAG) const {
-  if (SDValue FastLowered = LowerFastFDIV(Op, DAG))
+  const SDNodeFlags *Flags = Op->getFlags();
+  if ( Flags->hasAllowReciprocal())
+  { 
+    if (SDValue FastLowered = LowerFastFDIV(Op, DAG))
     return FastLowered;
+  }  
+
 
   // This uses v_rcp_f32 which does not handle denormals. Let this hit a
   // selection error for now rather than do something incorrect.

   SDValue LHS = Op.getOperand(0);
   SDValue RHS = Op.getOperand(1);
 
-  SDValue r1 = DAG.getNode(ISD::FABS, SL, MVT::f32, RHS);
+  // Generate the efficient but not precise fpdiv32 if fast_math is ON
+  if (EnableAMDGPUFastFDIV) {
 
-  const APFloat K0Val(BitsToFloat(0x6f800000));
-  const SDValue K0 = DAG.getConstantFP(K0Val, SL, MVT::f32);
+    SDValue r1 = DAG.getNode(ISD::FABS, SL, MVT::f32, RHS);
 
-  const APFloat K1Val(BitsToFloat(0x2f800000));
-  const SDValue K1 = DAG.getConstantFP(K1Val, SL, MVT::f32);
+    const APFloat K0Val(BitsToFloat(0x6f800000));
+    const SDValue K0 = DAG.getConstantFP(K0Val, SL, MVT::f32);
 
-  const SDValue One = DAG.getConstantFP(1.0, SL, MVT::f32);
+    const APFloat K1Val(BitsToFloat(0x2f800000));
+    const SDValue K1 = DAG.getConstantFP(K1Val, SL, MVT::f32);
 
-  EVT SetCCVT =
-      getSetCCResultType(DAG.getDataLayout(), *DAG.getContext(), MVT::f32);
+    const SDValue One = DAG.getConstantFP(1.0, SL, MVT::f32);
 
-  SDValue r2 = DAG.getSetCC(SL, SetCCVT, r1, K0, ISD::SETOGT);
+    EVT SetCCVT =
+        getSetCCResultType(DAG.getDataLayout(), *DAG.getContext(), MVT::f32);
 
-  SDValue r3 = DAG.getNode(ISD::SELECT, SL, MVT::f32, r2, K1, One);
+    SDValue r2 = DAG.getSetCC(SL, SetCCVT, r1, K0, ISD::SETOGT);
 
-  // TODO: Should this propagate fast-math-flags?
+    SDValue r3 = DAG.getNode(ISD::SELECT, SL, MVT::f32, r2, K1, One);
 
-  r1 = DAG.getNode(ISD::FMUL, SL, MVT::f32, RHS, r3);
+    // TODO: Should this propagate fast-math-flags?
 
-  SDValue r0 = DAG.getNode(AMDGPUISD::RCP, SL, MVT::f32, r1);
+    r1 = DAG.getNode(ISD::FMUL, SL, MVT::f32, RHS, r3);
 
-  SDValue Mul = DAG.getNode(ISD::FMUL, SL, MVT::f32, LHS, r0);
+    SDValue r0 = DAG.getNode(AMDGPUISD::RCP, SL, MVT::f32, r1);
 
-  return DAG.getNode(ISD::FMUL, SL, MVT::f32, r3, Mul);
+    SDValue Mul = DAG.getNode(ISD::FMUL, SL, MVT::f32, LHS, r0);
+
+    return DAG.getNode(ISD::FMUL, SL, MVT::f32, r3, Mul);
+  } else {
+    // Generates more precise fpdiv32.
+  const SDValue One = DAG.getConstantFP(1.0, SL, MVT::f32);
+  
+  SDVTList ScaleVT = DAG.getVTList(MVT::f32, MVT::i1);
+  
+  
+  SDValue d_scaled = DAG.getNode(AMDGPUISD::DIV_SCALE, SL, ScaleVT, RHS, RHS, LHS);
+  SDValue n_scaled = DAG.getNode(AMDGPUISD::DIV_SCALE, SL, ScaleVT, LHS, RHS, LHS);
+  
+  SDValue approx_rcp = DAG.getNode(AMDGPUISD::RCP, SL, MVT::f32, d_scaled);
+  
+  SDValue NegDivScale0 = DAG.getNode(ISD::FNEG, SL, MVT::f32, d_scaled);
+  
+  SDValue Fma0 = DAG.getNode(ISD::FMA, SL, MVT::f32, NegDivScale0, approx_rcp, One);
+  SDValue Fma1 = DAG.getNode(ISD::FMA, SL, MVT::f32, Fma0, approx_rcp, approx_rcp);
+  
+  SDValue Mul = DAG.getNode(ISD::FMUL, SL, MVT::f32, n_scaled, Fma1);
+  
+  SDValue Fma2 = DAG.getNode(ISD::FMA, SL, MVT::f32, NegDivScale0, Mul, n_scaled);
+  SDValue Fma3 = DAG.getNode(ISD::FMA, SL, MVT::f32, Fma2, Fma1, Mul);
+  SDValue Fma4 = DAG.getNode(ISD::FMA, SL, MVT::f32, NegDivScale0, Fma3, n_scaled);
+  
+  SDValue Scale = n_scaled.getValue(1);
+  SDValue Fmas = DAG.getNode(AMDGPUISD::DIV_FMAS, SL, MVT::f32, Fma4, Fma1, Fma3, Scale);
+  
+  return DAG.getNode(AMDGPUISD::DIV_FIXUP, SL, MVT::f32, Fmas, RHS, LHS);
+  }
 }
 
 SDValue SITargetLowering::LowerFDIV64(SDValue Op, SelectionDAG &DAG) const {