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

Reply via email to