This revision was automatically updated to reflect the committed changes.
Closed by commit rL333814: This diff includes changes for supporting the
following types. (authored by leonardchan, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
leonardchan added a comment.
Hi all, I'll be attempting to commit this patch around 6pm PT today unless
anyone has any more comments on this specific patch. Any other suggestions
regarding potential design changes can be discussed in future patches since
this is only the first of many.
ebevhan added inline comments.
Comment at: lib/Basic/TargetInfo.cpp:45
+ AccumWidth = AccumAlign = 32;
+ LongAccumWidth = LongAccumAlign = 64;
SuitableAlign = 64;
rsmith wrote:
> leonardchan wrote:
> > leonardchan wrote:
> > > ebevhan wrote:
> > > >
rsmith added inline comments.
Comment at: lib/Basic/TargetInfo.cpp:45
+ AccumWidth = AccumAlign = 32;
+ LongAccumWidth = LongAccumAlign = 64;
SuitableAlign = 64;
leonardchan wrote:
> leonardchan wrote:
> > ebevhan wrote:
> > > leonardchan wrote:
> > > >
leonardchan added inline comments.
Comment at: lib/Basic/TargetInfo.cpp:45
+ AccumWidth = AccumAlign = 32;
+ LongAccumWidth = LongAccumAlign = 64;
SuitableAlign = 64;
leonardchan wrote:
> ebevhan wrote:
> > leonardchan wrote:
> > > rsmith wrote:
> > > > jfb
leonardchan added inline comments.
Comment at: include/clang/Basic/TokenKinds.def:393
+// ISO/IEC JTC1 SC22 WG14 N1169 Extension
+KEYWORD(_Accum , KEYALL)
+
rsmith wrote:
> leonardchan wrote:
> > jfb wrote:
> > > ebevhan wrote:
> > > > I
leonardchan updated this revision to Diff 149305.
leonardchan marked 6 inline comments as done.
Repository:
rC Clang
https://reviews.llvm.org/D46084
Files:
include/clang-c/Index.h
include/clang/AST/ASTContext.h
include/clang/AST/BuiltinTypes.def
ebevhan added inline comments.
Comment at: lib/Basic/TargetInfo.cpp:45
+ AccumWidth = AccumAlign = 32;
+ LongAccumWidth = LongAccumAlign = 64;
SuitableAlign = 64;
rsmith wrote:
> jfb wrote:
> > This seems weird because Targets which don't have these values
rsmith accepted this revision.
rsmith added a comment.
Just minor comments. Feel free to commit after handling them.
Comment at: include/clang/Basic/LangOptions.def:301
+LANGOPT(FixedPointEnabled, 1, 0, "Fixed point types are enabled")
+
ebevhan wrote:
>
leonardchan added inline comments.
Comment at: include/clang/Basic/TokenKinds.def:393
+// ISO/IEC JTC1 SC22 WG14 N1169 Extension
+KEYWORD(_Accum , KEYALL)
+
jfb wrote:
> ebevhan wrote:
> > I believe that having KEYALL will enable the keyword
leonardchan updated this revision to Diff 149162.
Repository:
rC Clang
https://reviews.llvm.org/D46084
Files:
include/clang-c/Index.h
include/clang/AST/ASTContext.h
include/clang/AST/BuiltinTypes.def
include/clang/Basic/DiagnosticCommonKinds.td
include/clang/Basic/LangOptions.def
jfb added inline comments.
Comment at: include/clang/Basic/TokenKinds.def:393
+// ISO/IEC JTC1 SC22 WG14 N1169 Extension
+KEYWORD(_Accum , KEYALL)
+
ebevhan wrote:
> I believe that having KEYALL will enable the keyword even if
ebevhan added a comment.
Sorry for the late notice; I missed something.
Comment at: include/clang/Basic/TokenKinds.def:393
+// ISO/IEC JTC1 SC22 WG14 N1169 Extension
+KEYWORD(_Accum , KEYALL)
+
I believe that having KEYALL will enable the
leonardchan added a comment.
Hi all, I think I've addressed all comments on this patch and will be
committing tomorrow morning unless anyone has any more comments they'd like to
bring up.
Repository:
rC Clang
https://reviews.llvm.org/D46084
leonardchan marked 2 inline comments as done.
leonardchan added inline comments.
Comment at: include/clang/Driver/Options.td:882
+def enable_fixed_point : Flag<["-", "--"], "enable-fixed-point">,
Group,
+ Flags<[CC1Option]>, HelpText<"Enable fixed
leonardchan updated this revision to Diff 148637.
leonardchan added a comment.
Changed flag names
Repository:
rC Clang
https://reviews.llvm.org/D46084
Files:
include/clang-c/Index.h
include/clang/AST/ASTContext.h
include/clang/AST/BuiltinTypes.def
ebevhan added a comment.
With the exception of the two inline comments, this looks good to me now!
Comment at: include/clang/Basic/LangOptions.def:301
+LANGOPT(FixedPointEnabled, 1, 0, "Fixed point types are enabled")
+
Just a minor nit... The 'Enabled' is
leonardchan added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:382
+// enough bits to fit the minumum.
+if (getIntWidth() < MinSignedAccumDataBits)
+ return getLongWidth();
ebevhan wrote:
> I'm not sure I agree with this
leonardchan updated this revision to Diff 148506.
leonardchan added a comment.
Re-added individual getters/members for _Accum types
Repository:
rC Clang
https://reviews.llvm.org/D46084
Files:
include/clang-c/Index.h
include/clang/AST/ASTContext.h
include/clang/AST/BuiltinTypes.def
ebevhan added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:382
+// enough bits to fit the minumum.
+if (getIntWidth() < MinSignedAccumDataBits)
+ return getLongWidth();
I'm not sure I agree with this interpretation. It's simply not
leonardchan added a comment.
In https://reviews.llvm.org/D46084#374, @jfb wrote:
> Can you also add a test for `_Bool _Accum`.
>
> Also, `-enable-fixed-point -x c++` failing.
.
Done. Also the failing c++ case is under `test/Frontend/fixed_point_errors.cpp`
Repository:
rC Clang
leonardchan updated this revision to Diff 148481.
leonardchan marked 2 inline comments as done.
leonardchan added a comment.
- Added test case for `_Bool _Accum`
- Getters for the `_Accum` bit widths return values for their corresponding
integral types (ie. `sizeof(short _Accum) ==
jfb added a comment.
Can you also add a test for `_Bool _Accum`.
Also, `-enable-fixed-point -x c++` failing.
Comment at: lib/AST/ExprConstant.cpp:7361
+case BuiltinType::ULongAccum:
+ // GCC does not cover FIXED_POINT_TYPE in it's switch stmt and defaults
to
+
leonardchan updated this revision to Diff 148452.
Repository:
rC Clang
https://reviews.llvm.org/D46084
Files:
include/clang-c/Index.h
include/clang/AST/ASTContext.h
include/clang/AST/BuiltinTypes.def
include/clang/Basic/DiagnosticCommonKinds.td
include/clang/Basic/LangOptions.def
leonardchan marked 6 inline comments as done.
leonardchan added inline comments.
Comment at: lib/CodeGen/ItaniumCXXABI.cpp:2684
// Types added here must also be added to EmitFundamentalRTTIDescriptors.
switch (Ty->getKind()) {
rsmith wrote:
> Note this
leonardchan updated this revision to Diff 148445.
leonardchan added a comment.
- Reverted changes involving name mangling since we will only support c++ for
now. Will concurrently raise an issue on
https://github.com/itanium-cxx-abi/cxx-abi/ to get characters for name mangling.
- Added a flag
sammccall added inline comments.
Comment at: lib/Index/USRGeneration.cpp:691
+case BuiltinType::ULongAccum:
+ llvm_unreachable("No USR name mangling for fixed point types.");
case BuiltinType::Float16:
leonardchan wrote:
> rsmith wrote:
rjmccall added inline comments.
Comment at: include/clang/Basic/DiagnosticCommonKinds.td:172
+def err_fixed_point_only_allowed_in_c : Error<
+ "Fixed point types are only allowed in C">;
leonardchan wrote:
> leonardchan wrote:
> > leonardchan wrote:
> > >
leonardchan added a comment.
After further discussion, we think the best approach for now would be only
supporting fixed point types in C, then go back and support C++ once there is a
standardized way for mangling the fixed point types under itanium.
Repository:
rC Clang
leonardchan added inline comments.
Comment at: include/clang/Basic/DiagnosticCommonKinds.td:172
+def err_fixed_point_only_allowed_in_c : Error<
+ "Fixed point types are only allowed in C">;
leonardchan wrote:
> leonardchan wrote:
> > rsmith wrote:
> > >
jfb added a comment.
> Actually, scratch that. We will be enabling it since GCC does. Will update
> this and other relevant C++ related code appropriately.
Could you also add tests which mix _Accum with volatile, _Atomic, _Complex,
constexpr, inline?
Repository:
rC Clang
leonardchan added inline comments.
Comment at: include/clang/Basic/DiagnosticCommonKinds.td:172
+def err_fixed_point_only_allowed_in_c : Error<
+ "Fixed point types are only allowed in C">;
leonardchan wrote:
> rsmith wrote:
> > Diagnostics should not be
leonardchan added a subscriber: sammccall.
leonardchan added inline comments.
Comment at: include/clang/Basic/DiagnosticCommonKinds.td:172
+def err_fixed_point_only_allowed_in_c : Error<
+ "Fixed point types are only allowed in C">;
rsmith wrote:
>
ebevhan added inline comments.
Comment at: lib/AST/ASTContext.cpp:1775
+case BuiltinType::UShortAccum:
Width = Target->getShortWidth();
Align = Target->getShortAlign();
Please give the types their own width and alignment accessors/variables in
rjmccall added a comment.
There are at least three good reasons to make sure this can enabled/disabled by
a flag:
- We have to anticipate that introducing new keywords will cause some
compatibility problems. New language standards that introduce new keywords can
be disabled using `-std=`.
jfb added a comment.
This is on by default for any version of C? AFAICK `_Accum` isn't on the C17
draft that I have, I'd expect to have to specify a command-line flag pertaining
to TR 18037 to get this. At a minimum I'd be OK having it with the GNU variant
of C, but not the `__ANSI_C__` one.
rjmccall added a reviewer: akyrtzi.
rjmccall added a comment.
CC: Argyrios for the USR question.
Repository:
rC Clang
https://reviews.llvm.org/D46084
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
rsmith added subscribers: echristo, dblaikie, rjmccall.
rsmith added inline comments.
Comment at: include/clang/Basic/DiagnosticCommonKinds.td:172
+def err_fixed_point_only_allowed_in_c : Error<
+ "Fixed point types are only allowed in C">;
Diagnostics should
leonardchan updated this revision to Diff 148148.
leonardchan added a comment.
pulled changes from source tree
Repository:
rC Clang
https://reviews.llvm.org/D46084
Files:
include/clang-c/Index.h
include/clang/AST/ASTContext.h
include/clang/AST/BuiltinTypes.def
leonardchan updated this revision to Diff 148121.
Repository:
rC Clang
https://reviews.llvm.org/D46084
Files:
include/clang-c/Index.h
include/clang/AST/ASTContext.h
include/clang/AST/BuiltinTypes.def
include/clang/Basic/DiagnosticCommonKinds.td
include/clang/Basic/Specifiers.h
leonardchan updated this revision to Diff 148117.
leonardchan marked an inline comment as done.
Repository:
rC Clang
https://reviews.llvm.org/D46084
Files:
include/clang-c/Index.h
include/clang/AST/ASTContext.h
include/clang/AST/BuiltinTypes.def
leonardchan marked 2 inline comments as done.
leonardchan added inline comments.
Comment at: lib/Index/USRGeneration.cpp:731
+
+ if (c == '~') {
+switch (BT->getKind()) {
jakehehrlich wrote:
> You can make the 'c' a Twine instead. That will let you
leonardchan updated this revision to Diff 148025.
Repository:
rC Clang
https://reviews.llvm.org/D46084
Files:
include/clang-c/Index.h
include/clang/AST/ASTContext.h
include/clang/AST/BuiltinTypes.def
include/clang/Basic/DiagnosticCommonKinds.td
include/clang/Basic/Specifiers.h
jakehehrlich added a comment.
LGTM with a couple additions.
Comment at: lib/CodeGen/CodeGenTypes.cpp:448
+case BuiltinType::ULongAccum:
ResultType = llvm::IntegerType::get(getLLVMContext(),
static_cast(Context.getTypeSize(T)));
leonardchan updated this revision to Diff 147560.
Repository:
rC Clang
https://reviews.llvm.org/D46084
Files:
include/clang-c/Index.h
include/clang/AST/ASTContext.h
include/clang/AST/BuiltinTypes.def
include/clang/Basic/DiagnosticCommonKinds.td
include/clang/Basic/Specifiers.h
leonardchan updated this revision to Diff 147549.
Repository:
rC Clang
https://reviews.llvm.org/D46084
Files:
include/clang-c/Index.h
include/clang/AST/ASTContext.h
include/clang/AST/BuiltinTypes.def
include/clang/Basic/DiagnosticCommonKinds.td
include/clang/Basic/Specifiers.h
leonardchan updated this revision to Diff 147406.
leonardchan marked an inline comment as done.
leonardchan added a comment.
Undid git-clang-formatting on ASTBitcodes.h
Repository:
rC Clang
https://reviews.llvm.org/D46084
Files:
include/clang-c/Index.h
include/clang/AST/ASTContext.h
leonardchan marked 2 inline comments as done.
leonardchan added inline comments.
Comment at: lib/Index/USRGeneration.cpp:691
+case BuiltinType::ULongAccum:
+ llvm_unreachable("No USR name mangling for fixed point types.");
case BuiltinType::Float16:
leonardchan updated this revision to Diff 147400.
leonardchan added a comment.
Added break. We still assign `Result` since it cannot be null at the end of the
switch stmt, though the value doesn't matter.
Added character `~` to indicate fixed point type followed by string detailing
the type. I
leonardchan updated this revision to Diff 147386.
leonardchan added a comment.
Ran git-clang-tidy on all affected files
Repository:
rC Clang
https://reviews.llvm.org/D46084
Files:
include/clang-c/Index.h
include/clang/AST/ASTContext.h
include/clang/AST/BuiltinTypes.def
phosek added inline comments.
Comment at: lib/Index/USRGeneration.cpp:691
+case BuiltinType::ULongAccum:
+ llvm_unreachable("No USR name mangling for fixed point types.");
case BuiltinType::Float16:
We need some solution for fixed point
51 matches
Mail list logo
