ebevhan added a comment.
In https://reviews.llvm.org/D53738#1279069, @rjmccall wrote:
> > The important difference would be that we separate the semantics of
> > performing the conversion and the operation. I understand that adding a new
> > type could be a bit more involved than baking the conversion into the
> > operator, but I really do not enjoy seeing so much implicit,
> > implementation-specific logic encapsulated in the same AST element. Anyone
> > who wants to look at BinaryOperators with fixed-point types will need to
> > know all of the details of how the finding the common type and conversion
> > is done, rather than simply "it does an addition".
>
> It's not just that adding a new type is "involved", it's that it's a bad
> solution. Those types can't actually be expressed in the language, and
> changing the type system to be able to express them is going to lead to a lot
> of pain elsewhere.
I did figure that it might be a bit of work to adapt other parts of the code to
handle the new type, but I just prefer separating the concerns and being
explicit about what work is performed. To me, the clearest way of doing that is
by handling the conversions explicitly in the AST, and separately from the
operators themselves. Also, not being able to deal in QualTypes for the common
full precision type handling means that reusing the operator handling code in
Sema won't be easy, or even possible. It would have to be computed in
CreateBuiltinBinOp, since it's not possible to forward anything but QualType
from the CheckXXXOperands functions.
If you don't think it's a good idea I'll concede, but then there's the question
of how to get the full precision semantics into the operator (if we store it
there). I guess the most straightforward path is something like:
- In CreateBuiltinBinOp, do the normal Check function to get the result type
- If the result is a fixed-point type, go into a separate code path
- Ask a method for the common FixedPointSemantics of the given LHS and RHS
- Build the correct BinaryOperator subclass.
I need to think about this to see if our downstream implementation can be
adapted to work with this design.
Compound assignments are already their own subclass, though. Unless the full
precision semantic information is simply baked into the regular BinaryOperator,
it would require two new subclasses: one for normal full precision operators
and one for compound ones? Wouldn't adding this require new hooks and code
paths in visitors, even though there's really nothing different about the
operator?
The type information that CompoundAssignOperator stores today is rather similar
to what a full precision operator would need, though it would need to store a
FixedPointSemantics instead.
---
I do have more comments on the code at the moment, but I'm holding off a bit on
the review while we iron out some of these details.
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3156
+/// the type is an integer, the scale is zero.
+static void GetFixedPointAttributes(ASTContext &Ctx, QualType Ty,
+ unsigned &Width, unsigned &Scale,
----------------
This function should not be necessary. Instead, add to FixedPointSemantics:
* `static FixedPointSemantics GetIntegerSemantics(unsigned Width, bool
IsSigned)` to get an FPS for a specific integer width and signedness
(width=Width, scale=0, sat=false, signed=IsSigned, padding=false)
* `FixedPointSemantics getCommonSemantics(const FixedPointSemantics &Other)` to
get an FPS for the common full precision semantic between this FPS and another
one
================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3197
+ GetFixedPointAttributes(Ctx, RHSTy, RHSWidth, RHSScale, RHSSign);
+ GetFixedPointAttributes(Ctx, ResultTy, ResultWidth, ResultScale, ResultSign);
+
----------------
I think all of this (or at least something equivalent to it) should be
calculated in Sema, not in CodeGen.
If we go with the idea of storing the full precision semantics in the operator,
all the code in CodeGen should have to do is call EmitFixedPointConversion on
the LHS and RHS with the FixedPointSemantics given by the operator. Same for
converting back after the operation is performed.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:1310
+/// For a given fixed point type, return it's signed equivalent.
+static QualType GetCorrespondingSignedFixedPointType(ASTContext &Ctx,
+ QualType Ty) {
----------------
Maybe this should be a method on ASTContext itself? It's probably useful in
other cases.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:9219
+ else if (RHS.get()->getType()->isFixedPointType())
+ compType = FixedPointConversions(RHS, LHS, CompLHSTy);
+ else
----------------
I think this 'commutativity' should be handled inside the function rather than
here.
Repository:
rC Clang
https://reviews.llvm.org/D53738
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
