This revision was automatically updated to reflect the committed changes.
Closed by commit rL344530: [Fixed Point Arithmetic] FixedPointCast (authored by
leonardchan, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
leonardchan updated this revision to Diff 169714.
leonardchan marked an inline comment as done.
Repository:
rC Clang
https://reviews.llvm.org/D50616
Files:
include/clang/AST/OperationKinds.def
include/clang/AST/Type.h
include/clang/Basic/DiagnosticCommonKinds.td
lib/AST/Expr.cpp
ebevhan added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:1019
+ assert(!SrcType->isFixedPointType() && !DstType->isFixedPointType() &&
+ "Use the TargetCodeGenInfo::emitFixedPoint family functions for "
+ "handling conversions involving fixed
rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.
LGTM.
Repository:
rC Clang
https://reviews.llvm.org/D50616
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
leonardchan updated this revision to Diff 169500.
leonardchan added a comment.
- Removed target hook
Repository:
rC Clang
https://reviews.llvm.org/D50616
Files:
include/clang/AST/OperationKinds.def
include/clang/AST/Type.h
include/clang/Basic/DiagnosticCommonKinds.td
leonardchan updated this revision to Diff 169495.
Repository:
rC Clang
https://reviews.llvm.org/D50616
Files:
include/clang/AST/OperationKinds.def
include/clang/AST/Type.h
include/clang/Basic/DiagnosticCommonKinds.td
lib/AST/Expr.cpp
lib/AST/ExprConstant.cpp
lib/AST/Type.cpp
ebevhan added a comment.
I agree with John, after that I think it's fine for me.
Repository:
rC Clang
https://reviews.llvm.org/D50616
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
rjmccall added a comment.
In https://reviews.llvm.org/D50616#1259769, @leonardchan wrote:
> @ebevhan @rjmccall Seeing that the saturation intrinsic in
> https://reviews.llvm.org/D52286 should be put on hold for now, would it be
> fine to submit this patch as is? Then if the intrinsic is
leonardchan added a comment.
@ebevhan @rjmccall Seeing that the saturation intrinsic in
https://reviews.llvm.org/D52286 should be put on hold for now, would it be fine
to submit this patch as is? Then if the intrinsic is finished, come back to
this and update this patch to use the intrinsic?
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:419
+ case CK_LValueBitCast:
+ case CK_FixedPointCast: {
state =
a.sidorin wrote:
> Should we consider this construction as unsupported rather than supported as
>
a.sidorin added subscribers: NoQ, a.sidorin.
a.sidorin added a comment.
Accidentally noticed about this review via cfe-commits. @NoQ, the change in the
ExprEngine looks a bit dangerous to me. Could you please check?
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:419
+
rjmccall added a comment.
In https://reviews.llvm.org/D50616#1206181, @leonardchan wrote:
> I made a post on llvm-dev
> (http://lists.llvm.org/pipermail/llvm-dev/2018-August/125433.html) to see if
> other people have opinions on this. In the meantime, do you think I should
> make a separate
leonardchan added a comment.
I made a post on llvm-dev
(http://lists.llvm.org/pipermail/llvm-dev/2018-August/125433.html) to see if
other people have opinions on this. In the meantime, do you think I should make
a separate patch that moves this into an LLVM intrinsic function?
Repository:
ebevhan added a comment.
In https://reviews.llvm.org/D50616#1204588, @leonardchan wrote:
> Would it be simpler instead just to have the logic contained in the virtual
> function for `TargetCodeGenInfo` as opposed to returning `nullptr` since any
> custom target will end up overriding it anyway
rjmccall added a comment.
In https://reviews.llvm.org/D50616#1203772, @lebedev.ri wrote:
> In https://reviews.llvm.org/D50616#1203751, @rjmccall wrote:
>
> > In https://reviews.llvm.org/D50616#1203692, @ebevhan wrote:
> >
> > >
> >
> >
> > Has anyone actually asked LLVM whether they would accept
rjmccall added a comment.
>> Has anyone actually asked LLVM whether they would accept fixed-point types
>> into IR? I'm just a frontend guy, but it seems to me that there are
>> advantages to directly representing these operations in a portable way even
>> if there are no in-tree targets
leonardchan added a comment.
In https://reviews.llvm.org/D50616#1203692, @ebevhan wrote:
> In https://reviews.llvm.org/D50616#1203446, @leonardchan wrote:
>
> > Sorry I forgot to address this also. Just to make sure I understand this
> > correctly since I haven't used these before: target hooks
leonardchan updated this revision to Diff 161298.
leonardchan marked 4 inline comments as done.
Repository:
rC Clang
https://reviews.llvm.org/D50616
Files:
include/clang/AST/OperationKinds.def
include/clang/AST/Type.h
include/clang/Basic/DiagnosticCommonKinds.td
lib/AST/Expr.cpp
lebedev.ri added a comment.
In https://reviews.llvm.org/D50616#1203751, @rjmccall wrote:
> In https://reviews.llvm.org/D50616#1203692, @ebevhan wrote:
>
> >
>
>
> Has anyone actually asked LLVM whether they would accept fixed-point types
> into IR? I'm just a frontend guy, but it seems to me
rjmccall added a comment.
In https://reviews.llvm.org/D50616#1203692, @ebevhan wrote:
> In https://reviews.llvm.org/D50616#1203446, @leonardchan wrote:
>
> > Sorry I forgot to address this also. Just to make sure I understand this
> > correctly since I haven't used these before: target hooks
ebevhan added a comment.
In https://reviews.llvm.org/D50616#1203446, @leonardchan wrote:
> Sorry I forgot to address this also. Just to make sure I understand this
> correctly since I haven't used these before: target hooks are essentially for
> emitting target specific code for some
rjmccall added a comment.
In https://reviews.llvm.org/D50616#1203446, @leonardchan wrote:
> In https://reviews.llvm.org/D50616#1202034, @ebevhan wrote:
>
> > I think I've mentioned this before, but I would like to discuss the
> > possibility of adding a target hook(s) for some of these
leonardchan added a comment.
In https://reviews.llvm.org/D50616#1202034, @ebevhan wrote:
> I think I've mentioned this before, but I would like to discuss the
> possibility of adding a target hook(s) for some of these operations
> (conversions, arithmetic when it comes). Precisely matching the
leonardchan added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:331
+ SourceLocation Loc);
+
/// Emit a conversion from the specified complex type to the specified
ebevhan wrote:
> What's the plan for the other
ebevhan added a comment.
I think I've mentioned this before, but I would like to discuss the possibility
of adding a target hook(s) for some of these operations (conversions,
arithmetic when it comes). Precisely matching the emitted saturation operation
is virtually impossible for a target.
leonardchan added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:1034
+ if (DstFPSema.isSaturated() &&
+ (CGF.getContext().getCorrespondingSaturatedType(SrcTy) != DstTy)) {
+auto Mask = APInt::getBitsSetFrom(
rjmccall wrote:
> leonardchan
leonardchan updated this revision to Diff 160964.
leonardchan marked 7 inline comments as done.
leonardchan added a comment.
- Reworked logic for saturation
Repository:
rC Clang
https://reviews.llvm.org/D50616
Files:
include/clang/AST/OperationKinds.def
include/clang/AST/Type.h
rjmccall added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:1042
+std::min(DstScale + DstFPSema.getIntegralBits(), ResultWidth));
+Value *Zero = ConstantInt::get(CGF.getLLVMContext(), APInt(ResultWidth,
0));
+
You can just pass 0 here
leonardchan updated this revision to Diff 160884.
leonardchan marked 3 inline comments as done.
leonardchan added a comment.
- Added check for if we should check for saturation when converting to a
saturated fixed point type.
- Replaced `llvm_unreachable()`s with temporary diagnostic to be
leonardchan added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:1016
+ if (DstScale > SrcScale) {
+// Need to allocate space before shifting left
+ResultWidth = SrcWidth + DstScale - SrcScale;
rjmccall wrote:
> In IR, this isn't really
rjmccall added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:1016
+ if (DstScale > SrcScale) {
+// Need to allocate space before shifting left
+ResultWidth = SrcWidth + DstScale - SrcScale;
In IR, this isn't really "allocating" space.
leonardchan created this revision.
leonardchan added reviewers: ebevhan, phosek, mcgrathr, jakehehrlich.
leonardchan added a project: clang.
This patch is a part of https://reviews.llvm.org/D48456 in an attempt to split
them up. This contains the code for casting between fixed point types and
32 matches
Mail list logo
