[PATCH] D134859: [clang][Interp] Implement basic support for floating point values
Timm Bäder via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 18 23:45:13 PDT 2022
tbaeder added inline comments.
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:144
+
+ if (!this->emitCast(*FromT, PT_Float, CE))
+ return false;
----------------
sepavloff wrote:
> Does this two-stage conversion make sense? In contrast to things like `PT_Sint8` `PT_Float` is not a real type, it designates a set (potentially open) of all floating-point types. What is the meaning of this conversion? Why `emitCastFP` is not enough?
>
> BTW which classes implement `emitCast` and `emitCastFP`? Usually such casts depend on rounding mode, how these methods get it?
I think we could do this in `CastFP`, yes. But what happens right now is that we use `Floating::from()` with, e.g. a int32_t, which just does `APFloat(int32_t)`, which I think results in float semantics. So if we're instead casting from int to double, the `CastFP` is needed.
`CastFP` is implemented below in this diff and uses `Floating::toSemantics()`. `Cast` is using `ToT::from(FromT)`, so uses `Floating::from()` to do the cast.
================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:479
+ case PT_Float:
+ return this->emitZeroFloat(E);
}
----------------
sepavloff wrote:
> Should this method get floating-point semantic as a parameter?
Yes I think that would be alright, we could just get the right semantics in this function.
================
Comment at: clang/lib/AST/Interp/Floating.h:37
+ /// Zero-initializes a Floating.
+ Floating() : F(0.0f) {}
+ Floating(APFloat F) : F(F) {}
----------------
sepavloff wrote:
> This constructor creates a value of particular floating-point type, which generally is not compatible with floating-point values of other types. Does it need a semantic argument?
That's a good question. I think generally you're right but I don't know where this constructor is used at all right now. Might just be dead code.
================
Comment at: clang/lib/AST/Interp/Floating.h:54-62
+ explicit operator int8_t() const { return toAPSInt().getExtValue(); }
+ explicit operator uint8_t() const { return toAPSInt().getExtValue(); }
+ explicit operator int16_t() const { return toAPSInt().getExtValue(); }
+ explicit operator uint16_t() const { return toAPSInt().getExtValue(); }
+ explicit operator int32_t() const { return toAPSInt().getExtValue(); }
+ explicit operator uint32_t() const { return toAPSInt().getExtValue(); }
+ explicit operator int64_t() const { return toAPSInt().getExtValue(); }
----------------
sepavloff wrote:
> Conversions to integers are bitcasts+truncation but the conversion to bool is different. What semantics have these operations?
They are basically used for casting. I didn't mean to make the bool implementation different, just seemed to make sense to me to do it this way.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134859/new/
https://reviews.llvm.org/D134859
More information about the cfe-commits
mailing list