This is an archive of the discontinued LLVM Phabricator instance.

[APFloat] Make APFloat an interface class to the internal IEEEFloat. NFC.
ClosedPublic

Authored by timshen on Oct 12 2016, 4:07 PM.

Details

Summary

The intention is to make APFloat an interface class, so that later I can add a second implementation class DoubleAPFloat to correctly implement PPCDoubleDouble semantic. The interface of IEEEFloat is not public, and can be simplified (currently it's exactly the same as the old APFloat), but that (if exists) belongs to a separate patch.

DoubleAPFloat should look like:

class DoubleAPFloat {
  const fltSemantics *Semantics;
  std::unique_ptr<APFloat[]> APFloats;  // Two heap-allocated APFloats.
};

There is no functional change, nor public interface change.

Event Timeline

timshen updated this revision to Diff 74453.Oct 12 2016, 4:07 PM
timshen retitled this revision from to [APFloat] Make APFloat an interface class to the internal IEEEFloat. NFC..
timshen updated this object.
timshen added a subscriber: llvm-commits.
timshen updated this object.Oct 12 2016, 4:09 PM
timshen updated this object.
hfinkel edited edge metadata.Oct 12 2016, 4:19 PM

To be clear, you plan is to later change:

union {
  const fltSemantics *semantics;
  IEEEFloat IEEE;
};

to be something like:

union {
  const fltSemantics *semantics;
  IEEEFloat IEEE;
  PPCDoubleDouble PPCDD;
};

and then make:

opStatus divide(const APFloat &RHS, roundingMode RM) {
  return IEEE.divide(RHS.IEEE, RM);
}

into:

opStatus divide(const APFloat &RHS, roundingMode RM) {
  return isPPCDoubleDouble() ? PPCDD.divide(RHS, RM) : IEEE.divide(RHS, RM);
}

or similar?

To be clear, you plan is to later change:

union {
  const fltSemantics *semantics;
  IEEEFloat IEEE;
};

to be something like:

union {
  const fltSemantics *semantics;
  IEEEFloat IEEE;
  PPCDoubleDouble PPCDD;
};

and then make:

opStatus divide(const APFloat &RHS, roundingMode RM) {
  return IEEE.divide(RHS.IEEE, RM);
}

into:

opStatus divide(const APFloat &RHS, roundingMode RM) {
  return isPPCDoubleDouble() ? PPCDD.divide(RHS, RM) : IEEE.divide(RHS, RM);
}

or similar?

Yes, this is the plan.

To be clear, you plan is to later change:

union {
  const fltSemantics *semantics;
  IEEEFloat IEEE;
};

to be something like:

union {
  const fltSemantics *semantics;
  IEEEFloat IEEE;
  PPCDoubleDouble PPCDD;
};

and then make:

opStatus divide(const APFloat &RHS, roundingMode RM) {
  return IEEE.divide(RHS.IEEE, RM);
}

into:

opStatus divide(const APFloat &RHS, roundingMode RM) {
  return isPPCDoubleDouble() ? PPCDD.divide(RHS, RM) : IEEE.divide(RHS, RM);
}

or similar?

Yes, this is the plan.

Makes sense to me. Opinions from anyone else?

echristo edited edge metadata.Oct 13 2016, 9:52 PM

To be clear, you plan is to later change:

union {
  const fltSemantics *semantics;
  IEEEFloat IEEE;
};

to be something like:

union {
  const fltSemantics *semantics;
  IEEEFloat IEEE;
  PPCDoubleDouble PPCDD;
};

and then make:

opStatus divide(const APFloat &RHS, roundingMode RM) {
  return IEEE.divide(RHS.IEEE, RM);
}

into:

opStatus divide(const APFloat &RHS, roundingMode RM) {
  return isPPCDoubleDouble() ? PPCDD.divide(RHS, RM) : IEEE.divide(RHS, RM);
}

or similar?

Yes, this is the plan.

Makes sense to me. Opinions from anyone else?

Seems reasonable and the easiest solution here.

-eric

To be clear, you plan is to later change:

union {
  const fltSemantics *semantics;
  IEEEFloat IEEE;
};

to be something like:

union {
  const fltSemantics *semantics;
  IEEEFloat IEEE;
  PPCDoubleDouble PPCDD;
};

and then make:

opStatus divide(const APFloat &RHS, roundingMode RM) {
  return IEEE.divide(RHS.IEEE, RM);
}

into:

opStatus divide(const APFloat &RHS, roundingMode RM) {
  return isPPCDoubleDouble() ? PPCDD.divide(RHS, RM) : IEEE.divide(RHS, RM);
}

or similar?

Yes, this is the plan.

Makes sense to me. Opinions from anyone else?

hfinkel added inline comments.Oct 18 2016, 12:09 PM
llvm/include/llvm/ADT/APFloat.h
127

I think this should be named APFloatBase (e.g. we have DenseMapBase, SmallPtrSetImplBase, StringMapEntryBase).

timshen updated this revision to Diff 75071.Oct 18 2016, 2:11 PM

s/APFloatStatic/APFloatBase/

timshen marked an inline comment as done.Oct 18 2016, 2:12 PM
kbarton edited edge metadata.Oct 25 2016, 9:54 AM

Sounds good to me also!

kbarton accepted this revision.Oct 25 2016, 10:00 AM
kbarton edited edge metadata.

LGTM.
I went through this fairly carefully and couldn't spot anything wrong. I think there are some opportunities to make some of the methods const, but that would be outside the scope of this patch.

This revision is now accepted and ready to land.Oct 25 2016, 10:00 AM
This revision was automatically updated to reflect the committed changes.