Page MenuHomePhabricator

[Fixed Point Arithmetic] Fixed Point to Boolean Cast
ClosedPublic

Authored by leonardchan on Oct 15 2018, 6:09 PM.

Details

Summary

This patch is a part of https://reviews.llvm.org/D48456 in an attempt to split the casting logic up into smaller patches. This contains the code for casting from fixed point types to boolean types.

Diff Detail

Repository
rC Clang

Event Timeline

leonardchan created this revision.Oct 15 2018, 6:09 PM
ebevhan added inline comments.Oct 16 2018, 1:06 AM
clang/lib/CodeGen/CGExprScalar.cpp
1032 ↗(On Diff #169775)

Is this comment true? I don't think EmitFixedPointConversion does this.

Maybe I'm misinterpreting what it means.

2023 ↗(On Diff #169775)

This comment is a duplicate of the one in EmitScalarConversion.

clang/lib/Sema/Sema.cpp
537 ↗(On Diff #169775)

Put this on the line above directly after the case like the others.

leonardchan marked 3 inline comments as done.
leonardchan added inline comments.
clang/lib/CodeGen/CGExprScalar.cpp
1032 ↗(On Diff #169775)

Ah sorry, this was my bad. I briefly forgot that we intended for overflow into the bit to be undefined behavior. In that case, this comment doesn't make sense and should be removed, but we still shouldn't have to clear/check the padding bit in this case because we should assume it's zero'd.

So this refers to handling when the padding bit needs to be cleared since if we ever have an operation that reads the whole int of an underlying unsigned type. I accidentally wrote this comment thinking that it would be cleared/zero'd at some point when we decided before that overflow into this padding was just undefined behavior. Updated the comment to reflect this.

rjmccall added inline comments.Oct 18 2018, 11:48 AM
clang/lib/AST/ExprConstant.cpp
9599 ↗(On Diff #169863)

I know you haven't really done constant-evaluation yet, but I think you should at least be setting up operations like this correctly:

  1. There should be an EvaluateFixedPoint function that evaluates an expression as a fixed-point value, just like EvaluateFloat and the others, which can be structured to use FixedPointExprEvaluator.
  2. There should be a getBoolValue method on APFixedPoint.
clang/lib/CodeGen/CGExprScalar.cpp
2026 ↗(On Diff #169863)

Why are you pushing these casts through EmitScalarConversion when the cast kind already tells you which operation you're doing?

ebevhan added inline comments.Oct 22 2018, 7:17 AM
clang/lib/CodeGen/CGExprScalar.cpp
2026 ↗(On Diff #169863)

It could be useful to enable EmitScalarConversion to handle any of the conversions so it can be used in other contexts than expanding a cast.

leonardchan added inline comments.Oct 22 2018, 12:04 PM
clang/lib/CodeGen/CGExprScalar.cpp
2026 ↗(On Diff #169863)

It could be useful to enable EmitScalarConversion to handle any of the conversions so it can be used in other contexts than expanding a cast.

This was also the reason I did this since EmitScalarConversion seems to get called in many places as a generic dispatch for different conversions.

rjmccall accepted this revision.Oct 23 2018, 9:23 AM

LGTM.

clang/lib/CodeGen/CGExprScalar.cpp
2026 ↗(On Diff #169863)

I think we do that in a very restricted set of places, like compound assignment operators, and honestly they should be changed so that they store the final cast kind. But I suppose I can't ask you to fight the current system.

This revision is now accepted and ready to land.Oct 23 2018, 9:23 AM
leonardchan added inline comments.Oct 23 2018, 10:41 AM
clang/lib/AST/ExprConstant.cpp
9599 ↗(On Diff #169863)

I will add these in another patch since this has LGTM.

This revision was automatically updated to reflect the committed changes.