[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

I think the solution to a lot of diagnostic and behavior issues with address 
spaces is to remove predication on OpenCL. It's a bit odd to have a language 
feature that is enabled out of the box regardless of langoptions (address 
spaces) but won't actually work properly unless you're building OpenCL.




Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6943
+def err_typecheck_incompatible_conditional_pointer_operands : Error<
+  "unable to find common type between %0 and %1 for conditional operation">;
 

This error is very similar to the one in my first comment, `conditional 
operator with the second and third operands of type 
('__attribute__((address_space(1))) char *' and 
'__attribute__((address_space(2))) char *') which are pointers to 
non-overlapping address spaces`. It would normally be emitted at 6472, but 
won't be since OpenCL isn't enabled.



Comment at: test/Sema/conditional-expr.c:78
+   // expected-error@-1{{converting 
'__attribute__((address_space(2))) int *' to type 'void *' changes address 
space of pointer}}
+   // expected-error@-2{{converting 
'__attribute__((address_space(3))) int *' to type 'void *' changes address 
space of pointer}}
 

rjmccall wrote:
> leonardchan wrote:
> > rjmccall wrote:
> > > Also, these diagnostics seem wrong.  Where is `void *` coming from?
> > When dumping the AST this is what the resulting type is for the conditional 
> > expression already is if the operands are 2 pointers with different address 
> > spaces.
> > 
> > According to this comment, the reason seems to be because this is what GCC 
> > does:
> > 
> > ```
> >  6512 // In this situation, we assume void* type. No especially good
> >  6513 // reason, but this is what gcc does, and we do have to pick
> >  6514 // to get a consistent AST.
> > ```
> That makes sense in general, but in this case it's not a great diagnostic; we 
> should just emit an error when trying to pick a common type.
Is it possible that you are getting `void *` because we aren't running the 
qualifier removal at 6495? I don't think I've ever seen spurious `void *`'s 
show up in our downstream diagnostics.


Repository:
  rC Clang

https://reviews.llvm.org/D50278



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

[Bevin Hansson via Phabricator via cfe-commits]

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.




Comment at: lib/CodeGen/CGExprScalar.cpp:331
+  SourceLocation Loc);
+
   /// Emit a conversion from the specified complex type to the specified

What's the plan for the other conversions (int<->fix, float<->fix)? Functions 
for those as well?

What about `EmitScalarConversion`? If it cannot handle conversions of 
fixed-point values it should probably be made to assert, since it will likely 
mess up.



Comment at: lib/CodeGen/CGExprScalar.cpp:1017
+// Need to extend first before scaling up
+ResultWidth = SrcWidth + DstScale - SrcScale;
+llvm::Type *UpscaledTy = Builder.getIntNTy(ResultWidth);

It is definitely simpler to always do the 'upscale+resize/downscale, 
[saturate], resize' in the constant evaluation case, but when emitting IR it is 
not as efficient. There's no need to keep the upshifted bits if you aren't 
interested in saturation, so in the non-saturating case it is better to keep 
everything in the native type sizes with '[downscale], resize, [upscale]'. This 
is why there are a bunch of 'sext i32 to i33' in the test cases.

Perhaps something like
```
if !dst.saturating
  downscale if needed
  resize
  upscale if needed
  return 

old code here, minus the 'DstFPSema.isSaturated()' which is implied
```

It gives a bit of duplication (or at least similar code) but I think it's an 
important disambiguation to make.



Comment at: lib/CodeGen/CGExprScalar.cpp:1052
+} else if (IsSigned && !DstFPSema.isSigned()) {
+  Value *Zero =
+  ConstantInt::get(CGF.getLLVMContext(), APInt(ResultWidth, 0));

`ConstantInt::getNullValue`?



Comment at: lib/Sema/Sema.cpp:537
+  case Type::STK_FixedPoint:
+llvm_unreachable("Unknown cast from FixedPoint to boolean");
   }

Will we want to support this?


Repository:
  rC Clang

https://reviews.llvm.org/D50616



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

[Bevin Hansson via Phabricator via cfe-commits]

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 operations right? Does this mean that 
> the `EmitFixedPointConversion` function should be moved to a virtual method 
> under `TargetCodeGenInfo` that can be overridden and this is what get's 
> called instead during conversion?


Yes, the thought I had was to have a virtual function in TargetCodeGenInfo that 
would be called first thing in EmitFixedPointConversion, and if it returns 
non-null it uses that value instead. It's a bit unfortunate in this instance as 
the only thing that doesn't work for us is the saturation, but letting you 
configure *parts* of the emission is a bit too specific.

In https://reviews.llvm.org/D50616#1203635, @rjmccall wrote:

> If this is more than just a hobby — like if there's a backend that wants to 
> do serious work with matching fixed-point operations to hardware support — we 
> should just add real LLVM IR support for fixed-point types instead of adding 
> a bunch of frontend customization hooks.  I don't know what better 
> opportunity we're expecting might come along to justify that, and I don't 
> think it's some incredibly onerous task to add a new leaf to the LLVM type 
> hierarchy.  Honestly, LLVM programmers across the board have become far too 
> accustomed to pushing things opaquely through an uncooperative IR with an 
> obscure muddle of intrinsics.
>
> In the meantime, we can continue working on this code.  Even if there's 
> eventually real IR support for fixed-point types, this code will still be 
> useful; it'll just become the core of some legalization pass in the generic 
> backend.


It definitely is; our downstream target requires it. As far as I know there's 
no real use case upstream, which is why it's likely quite hard to add any 
extensions to LLVM proper. Our implementation is done through intrinsics rather 
than an extension of the type system, as fixed-point numbers are really just 
integers under the hood. We've always wanted to upstream our fixed-point 
support (or some reasonable adaptation of it) but never gotten the impression 
that it would be possible since there's no upstream user.

A mail I sent to the mailing list a while back has more details on our 
implementation, including what intrinsics we've added: 
http://lists.llvm.org/pipermail/cfe-dev/2018-May/058019.html We also have an 
LLVM pass that converts these intrinsics into pure IR, but it's likely that it 
won't work properly with some parts of this upstream design.

Leonard's original proposal for this work has always been Clang-centric and 
never planned for implementing anything on the LLVM side, so we need to make 
due with what we get, but we would prefer as small a diff from upstream as 
possible.




Comment at: lib/CodeGen/CGExprScalar.cpp:331
+  SourceLocation Loc);
+
   /// Emit a conversion from the specified complex type to the specified

leonardchan wrote:
> ebevhan wrote:
> > What's the plan for the other conversions (int<->fix, float<->fix)? 
> > Functions for those as well?
> > 
> > What about `EmitScalarConversion`? If it cannot handle conversions of 
> > fixed-point values it should probably be made to assert, since it will 
> > likely mess up.
> Ideally, my plan was to have separate functions for each cast since it seems 
> the logic for each of them is unique, other than saturation which may be 
> abstracted to its own function and be used by the others.
> 
> I wasn't sure if I should also place the logic for these casts in 
> `EmitScalarConversion` since `EmitScalarConversion` seemed pretty large 
> enough and thought it was easier if we instead had separate, self contained 
> functions for each fixed point conversion. But I'm open for hearing ideas on 
> different ways on implementing them.
> 
> Will add the assertion.
Yes, you have a point. Our EmitScalarConversion does all of the fixed-point 
stuff at once and it's a pretty big mess.

It might be a bit confusing for users if they use EmitScalarConversion, 
expecting it to work on fixed-point scalars as well but then it doesn't work 
properly.

So long as it asserts to tell them that, it should be fine but someone else 
might have an opinion on the expected behavior of the function.


Repository:
  rC Clang

https://reviews.llvm.org/D50616



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

[Bevin Hansson via Phabricator via cfe-commits]

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 and ideally not return 
> `nullptr`?


I guess that's also possible. I figured it would be clearer to have the generic 
logic in the more obvious location (CGExprScalar) and fall back to the custom 
handling if needed. Many of the handlers in TargetCodeGenInfo are empty.

> I haven't brought this up to llvm-dev, but the main reason for why we decided 
> to only implement this in clang was because we thought it would be a lot 
> harder pushing a new type into upstream llvm, especially since a lot of the 
> features can be supported on the frontend using clang's existing 
> infrastructure. We are open to adding fixed point types to LLVM IR in the 
> future though once all the frontend stuff is laid out and if the community is 
> willing to accept it.

I know there are multiple threads on llvm-dev asking about fixed-point support 
(some of them from us) and the verdict usually seems to be "no new types, use 
integers and intrinsics". This has worked fairly well for us, so it should be 
possible to adapt the design for upstream if there is a desire to have it.

I do not think a new type is required. As there are no in-tree architectures 
with native fixed-point support (as far as I know? Does AVR or ARM support it, 
perhaps? Probably not enough to cover the entire E-C spec, though.), there 
isn't really much to warrant adding a new type (or even new IR operators) for 
it. Furthermore, many of the operations on fixed-point are simple integer 
operations (except ones like saturation/saturating operations, multiplication, 
division etc) so those would be better off implemented in terms of those 
operators rather than inventing new ones that do pretty much the same thing.

In https://reviews.llvm.org/D50616#1204754, @rjmccall wrote:

> Very few things in LLVM actually try to exhaustively handle all operations.  
> There are a
>  couple of generic predicates on `Instruction` like `mayHaveSideEffects`, 
> there's
>  serialization/`.ll`-writing, and there's code-gen.  The first two are not 
> hard at all
>  to implement, and it'd be quite simple to write a legalization pass in 
> code-gen that
>  turns all these operations into integer operations and which could easily be 
> customized
>  to support targets that want to do something more precise.


I certainly support the idea of representing some (not all) fixed-point 
operations as intrinsics (or instructions, although I suspect that's more work 
than one might think) and converting them into integer instructions for targets 
which do not care to support the specific operation natively. A type is 
overkill; fixed-point values **are** 'bags of bits' like LLVM integers, just 
with a different interpretation of the bits. It's no different than the 
distinction between signed and unsigned, and we model that on the LLVM level 
through the operations rather than through the types.

Creating instruction/intrinsic definitions of fixed-point operations does come 
with problems. If an IR producer wants to emit fixed-point operations with 
slightly different semantics than what our instructions/intrinsics have, 
they're out of luck. It might also be a problem for targets that have 
fixed-point support, but cannot perform the operations in exactly the same 
manner as the IR operation specifies. Making the semantics of the operation too 
tight could cause this to happen, but making them too loose makes it hard to 
really do anything with it. This is another argument for frontend 
customization. Every operation also introduces a bit of optimization/codegen 
work, unlike pure integer operations which the passes already know.

Still, I think intrinsics for the complicated operations would provide a good 
balance.

> The advantages of having real IR support include:
> 
> - It's significant simpler for frontends.  All of this logic in Clang will 
> need to be reimplemented in any other frontend that wants to support 
> fixed-point types.

As I mentioned, this only simplifies things if the semantics of the operations 
work for the other frontend/language/target.

> 
> 
> - It's much easier to test.  The frontend needs to handle a lot of different 
> code patterns that ultimately turn into the same small set of basic 
> operations, like compound arithmetic and atomic operations and a million 
> different things that are supposed to trigger implicit conversions.  It's ver 
> frustrating to write tests for these things when constants aren't readable 
> and the generated IR for every operation is a ten-instruction sequence.

I think this is primarily a problem for saturating operations, as those require 
a whole bunch of conditional assignment. Nonsaturating multiplication is only 
about 5 inst

[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan created this revision.
ebevhan added reviewers: dergachev.a, dcoughlin.
Herald added subscribers: cfe-commits, a.sidorin, szepet, xazax.hun.
Herald added a reviewer: george.karpenkov.

RegionStoreManager::getSizeInElements used 'int' for size
calculations, and ProgramState::assumeInBound fell back
to 'int' as well for its index calculations. This causes
truncation for sufficiently large sizes/indexes.

Use a signed size_t and ArrayIndexTy respectively to
prevent these problems.


Repository:
  rC Clang

https://reviews.llvm.org/D46944

Files:
  lib/StaticAnalyzer/Core/ProgramState.cpp
  lib/StaticAnalyzer/Core/RegionStore.cpp
  test/Analysis/array-index.c


Index: test/Analysis/array-index.c
===
--- /dev/null
+++ test/Analysis/array-index.c
@@ -0,0 +1,21 @@
+// RUN: %clang_analyze_cc1 
-analyzer-checker=core,alpha.security.ArrayBound,alpha.unix.cstring.OutOfBounds 
-verify -Wno-implicit-function-declaration %s
+
+// expected-no-diagnostics
+
+#define SIZE 4294967296
+
+static unsigned size;
+static void * addr;
+static unsigned buf[SIZE];
+
+void fie() {
+  buf[SIZE-1] = 1;
+}
+
+void foo() {
+  memcpy(buf, addr, size);
+}
+
+void bar() {
+  memcpy(addr, buf, size);
+}
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1341,7 +1341,7 @@
   // If a variable is reinterpreted as a type that doesn't fit into a larger
   // type evenly, round it down.
   // This is a signed value, since it's used in arithmetic with signed indices.
-  return svalBuilder.makeIntVal(RegionSize / EleSize, false);
+  return svalBuilder.makeIntVal(RegionSize / EleSize, Ctx.getSignedSizeType());
 }
 
 
//===--===//
Index: lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- lib/StaticAnalyzer/Core/ProgramState.cpp
+++ lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -336,9 +336,8 @@
 
   // Get the offset: the minimum value of the array index type.
   BasicValueFactory &BVF = svalBuilder.getBasicValueFactory();
-  // FIXME: This should be using ValueManager::ArrayindexTy...somehow.
   if (indexTy.isNull())
-indexTy = Ctx.IntTy;
+indexTy = svalBuilder.getArrayIndexType();
   nonloc::ConcreteInt Min(BVF.getMinValue(indexTy));
 
   // Adjust the index.


Index: test/Analysis/array-index.c
===
--- /dev/null
+++ test/Analysis/array-index.c
@@ -0,0 +1,21 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.security.ArrayBound,alpha.unix.cstring.OutOfBounds -verify -Wno-implicit-function-declaration %s
+
+// expected-no-diagnostics
+
+#define SIZE 4294967296
+
+static unsigned size;
+static void * addr;
+static unsigned buf[SIZE];
+
+void fie() {
+  buf[SIZE-1] = 1;
+}
+
+void foo() {
+  memcpy(buf, addr, size);
+}
+
+void bar() {
+  memcpy(addr, buf, size);
+}
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1341,7 +1341,7 @@
   // If a variable is reinterpreted as a type that doesn't fit into a larger
   // type evenly, round it down.
   // This is a signed value, since it's used in arithmetic with signed indices.
-  return svalBuilder.makeIntVal(RegionSize / EleSize, false);
+  return svalBuilder.makeIntVal(RegionSize / EleSize, Ctx.getSignedSizeType());
 }
 
 //===--===//
Index: lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- lib/StaticAnalyzer/Core/ProgramState.cpp
+++ lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -336,9 +336,8 @@
 
   // Get the offset: the minimum value of the array index type.
   BasicValueFactory &BVF = svalBuilder.getBasicValueFactory();
-  // FIXME: This should be using ValueManager::ArrayindexTy...somehow.
   if (indexTy.isNull())
-indexTy = Ctx.IntTy;
+indexTy = svalBuilder.getArrayIndexType();
   nonloc::ConcreteInt Min(BVF.getMinValue(indexTy));
 
   // Adjust the index.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1344
   // This is a signed value, since it's used in arithmetic with signed indices.
-  return svalBuilder.makeIntVal(RegionSize / EleSize, false);
+  return svalBuilder.makeIntVal(RegionSize / EleSize, Ctx.getSignedSizeType());
 }

a.sidorin wrote:
> I think we should initialize SValBuilder::ArrayIndexTy with 
> getSignedSizeType() instead of LongLongTy and use 
> `svalBuilder.getArrayIndexType()` here instead.
I made the change, but it caused a spurious out of bounds warning in 
index-type.c for the 32-bit case. Making the type signed means that anything 
above MAX/2 will break, and the test uses arrays of that size.


Repository:
  rC Clang

https://reviews.llvm.org/D46944



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1344
   // This is a signed value, since it's used in arithmetic with signed indices.
-  return svalBuilder.makeIntVal(RegionSize / EleSize, false);
+  return svalBuilder.makeIntVal(RegionSize / EleSize, Ctx.getSignedSizeType());
 }

a.sidorin wrote:
> ebevhan wrote:
> > a.sidorin wrote:
> > > I think we should initialize SValBuilder::ArrayIndexTy with 
> > > getSignedSizeType() instead of LongLongTy and use 
> > > `svalBuilder.getArrayIndexType()` here instead.
> > I made the change, but it caused a spurious out of bounds warning in 
> > index-type.c for the 32-bit case. Making the type signed means that 
> > anything above MAX/2 will break, and the test uses arrays of that size.
> Hm, yes. ssize_t is 32-bit on 32-bit targets but our indices can exceed it. 
> Even if so, `svalBuilder.getArrayIndexType()` should be fine.
Well, the problem is that it's only enough for objects whose size is less than 
half of the range. If they're larger, the out of bounds calculation overflows 
and it thinks that we're trying to index outside the object (at a lower 
address).

The 64-bit test avoids this since its array is smaller. It's MAX/16 instead.


Repository:
  rC Clang

https://reviews.llvm.org/D46944



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan updated this revision to Diff 147738.
ebevhan edited the summary of this revision.
ebevhan added a comment.

Made ArrayIndexTy into ssize_t, consolidated the tests and fixed the test that 
was failing.


https://reviews.llvm.org/D46944

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  lib/StaticAnalyzer/Core/ProgramState.cpp
  lib/StaticAnalyzer/Core/RegionStore.cpp
  test/Analysis/index-type.c


Index: test/Analysis/index-type.c
===
--- test/Analysis/index-type.c
+++ test/Analysis/index-type.c
@@ -6,15 +6,34 @@
 
 #ifdef M32
 
-#define X86_ARRAY_SIZE (UINT_MAX/2 + 4)
+#define X86_ARRAY_SIZE (UINT_MAX/4 + 4)
 
 void testIndexTooBig() {
   char arr[X86_ARRAY_SIZE];
-  char *ptr = arr + UINT_MAX/2;
+  char *ptr = arr + UINT_MAX/4;
   ptr += 2;  // index shouldn't overflow
   *ptr = 42; // no-warning
 }
 
+#define SIZE 4294967296
+
+static unsigned size;
+static void * addr;
+static unsigned buf[SIZE];
+
+void testOutOfBounds() {
+  // not out of bounds
+  buf[SIZE-1] = 1; // no-warning
+}
+
+void testOutOfBoundsCopy1() {
+  memcpy(buf, addr, size); // no-warning
+}
+
+void testOutOfBoundsCopy2() {
+  memcpy(addr, buf, size); // no-warning
+}
+
 #else // 64-bit tests
 
 #define ARRAY_SIZE 0x1
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1341,7 +1341,8 @@
   // If a variable is reinterpreted as a type that doesn't fit into a larger
   // type evenly, round it down.
   // This is a signed value, since it's used in arithmetic with signed indices.
-  return svalBuilder.makeIntVal(RegionSize / EleSize, false);
+  return svalBuilder.makeIntVal(RegionSize / EleSize,
+svalBuilder.getArrayIndexType());
 }
 
 
//===--===//
Index: lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- lib/StaticAnalyzer/Core/ProgramState.cpp
+++ lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -336,9 +336,8 @@
 
   // Get the offset: the minimum value of the array index type.
   BasicValueFactory &BVF = svalBuilder.getBasicValueFactory();
-  // FIXME: This should be using ValueManager::ArrayindexTy...somehow.
   if (indexTy.isNull())
-indexTy = Ctx.IntTy;
+indexTy = svalBuilder.getArrayIndexType();
   nonloc::ConcreteInt Min(BVF.getMinValue(indexTy));
 
   // Adjust the index.
Index: include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
@@ -86,7 +86,7 @@
   ProgramStateManager &stateMgr)
   : Context(context), BasicVals(context, alloc),
 SymMgr(context, BasicVals, alloc), MemMgr(context, alloc),
-StateMgr(stateMgr), ArrayIndexTy(context.LongLongTy),
+StateMgr(stateMgr), ArrayIndexTy(context.getSignedSizeType()),
 ArrayIndexWidth(context.getTypeSize(ArrayIndexTy)) {}
 
   virtual ~SValBuilder() = default;


Index: test/Analysis/index-type.c
===
--- test/Analysis/index-type.c
+++ test/Analysis/index-type.c
@@ -6,15 +6,34 @@
 
 #ifdef M32
 
-#define X86_ARRAY_SIZE (UINT_MAX/2 + 4)
+#define X86_ARRAY_SIZE (UINT_MAX/4 + 4)
 
 void testIndexTooBig() {
   char arr[X86_ARRAY_SIZE];
-  char *ptr = arr + UINT_MAX/2;
+  char *ptr = arr + UINT_MAX/4;
   ptr += 2;  // index shouldn't overflow
   *ptr = 42; // no-warning
 }
 
+#define SIZE 4294967296
+
+static unsigned size;
+static void * addr;
+static unsigned buf[SIZE];
+
+void testOutOfBounds() {
+  // not out of bounds
+  buf[SIZE-1] = 1; // no-warning
+}
+
+void testOutOfBoundsCopy1() {
+  memcpy(buf, addr, size); // no-warning
+}
+
+void testOutOfBoundsCopy2() {
+  memcpy(addr, buf, size); // no-warning
+}
+
 #else // 64-bit tests
 
 #define ARRAY_SIZE 0x1
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1341,7 +1341,8 @@
   // If a variable is reinterpreted as a type that doesn't fit into a larger
   // type evenly, round it down.
   // This is a signed value, since it's used in arithmetic with signed indices.
-  return svalBuilder.makeIntVal(RegionSize / EleSize, false);
+  return svalBuilder.makeIntVal(RegionSize / EleSize,
+svalBuilder.getArrayIndexType());
 }
 
 //===--===//
Index: lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- li

[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

[Bevin Hansson via Phabricator via cfe-commits]

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 
TargetInfo and use those instead of reusing the existing ones.



Comment at: lib/Sema/SemaType.cpp:1395
+  case DeclSpec::TST_accum: {
+if (S.getLangOpts().CPlusPlus) {
+  S.Diag(DS.getTypeSpecTypeLoc(), diag::err_fixed_point_only_allowed_in_c);

This (and the rest of the fixed-point support) should be behind its own option. 
The error should reflect this as well.


Repository:
  rC Clang

https://reviews.llvm.org/D46084



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46960: [Fixed Point Arithmetic] Predefined Precision Macros

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: include/clang/Lex/Preprocessor.h:185
+  // Fractional bits of _Accum types
+  IdentifierInfo *Ident__SACCUM_FBIT__;// __SACCUM_FBIT__
+  IdentifierInfo *Ident__ACCUM_FBIT__; // __ACCUM_FBIT__

You should not reserve identifiers like this for built-in limit/precision 
macros. See InitPreprocessor.cpp to see how it's supposed to be done.

We have implemented all of these (at least for DSP-C, but it should not be 
difficult to port this to Embedded-C) in our downstream port. You will also 
need routines to print fixed-point numbers for the rest of the macros in 
7.18a.3. We can provide these patches on request.


Repository:
  rC Clang

https://reviews.llvm.org/D46960



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46911: [Fixed Point Arithmetic] Addition of the remaining fixed point types and their saturated equivalents

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:206
 
+def err_invalid_saturation_spec : Error<"'%0' cannot be saturated. Only _Fract 
and _Accum can.">;
 def err_invalid_sign_spec : Error<"'%0' cannot be signed or unsigned">;

This error should be reworded.

Perhaps `'_Sat' specifier is only valid on '_Fract' or '_Accum', not '%0'`.



Comment at: lib/Sema/DeclSpec.cpp:1139
 
+  bool is_fixed_point_type =
+  (TypeSpecType == TST_accum || TypeSpecType == TST_fract);

This variable name should probably be a different case.



Comment at: lib/Sema/SemaType.cpp:1430
 } else {
-  switch (DS.getTypeSpecWidth()) {
-case DeclSpec::TSW_short:
-  Result = Context.UnsignedShortAccumTy;
-  break;
-case DeclSpec::TSW_unspecified:
-  Result = Context.UnsignedAccumTy;
-  break;
-case DeclSpec::TSW_long:
-  Result = Context.UnsignedLongAccumTy;
-  break;
-case DeclSpec::TSW_longlong:
-  // TODO: Replace with diag
-  llvm_unreachable("Unable to specify long long as _Accum width");
-  break;
+  if (DS.getTypeSpecSign() != DeclSpec::TSS_unsigned) {
+switch (DS.getTypeSpecWidth()) {

You should probably use the 'getCorrespondingSaturatingType' function instead 
of disambiguating the whole thing here again.



Comment at: test/Frontend/fixed_point.c:45
+_Sat _Fract sat_fract;
+_Sat long _Fract sat_long_fract;
+

What about typedefs with the _Sat specifier on them?


Repository:
  rC Clang

https://reviews.llvm.org/D46911



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46915: [Fixed Point Arithmetic] Set Fixed Point Precision Bits and Create Fixed Point Literals

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

You should not define the fixed-point precision as compiler macros at build 
configure time. The number of fractional bits (the scaling factor) should be 
added to TargetInfo as target-configurable variables/accessors, and an accessor 
should be added to ASTContext (we call it 'getFixedPointScale') to fetch the 
scaling factor of arbitrary fixed-point types.

As I mentioned on the mailing list, we would also like to solve the problem 
with different scaling factors on the signed and unsigned _Fract types. I'm not 
sure what the best approach is, but the simplest solution could perhaps be 
enabled with a target flag.

If the flag is true, the scaling factor of the unsigned _Fract types is the 
same as the signed ones and the MSB is padding. If the flag is false, the 
scaling factor of the unsigned _Fract types is one greater than the signed 
types and all of the bits are used.

-

Overall, the literal parsing code seems very ad-hoc. Fixed-point values are 
just integers, it should be possible to process them in exact precision instead 
of relying on host system implementation details (using double) and floating 
point types.

I could provide patches from our downstream port, but there are a number of 
hurdles:

- DSP-C does a bunch of extra 'type promotion' when parsing fixed-point literals
- literals in DSP-C cannot contain the 'exponent notation' so the parsing 
routine cannot handle this
- the routine uses utility functions added to APInt in our LLVM branch

If you are interested I can share anyway so you can see how we have done it.




Comment at: include/clang/Lex/LiteralSupport.h:72
 
+  enum FixedPointType { FPT_UNSPECIFIED, FPT_ACCUM, FPT_FRACT };
+

Is there a reason this is not added as two fields 'isAccum' and 'isFract'?



Comment at: include/clang/Lex/LiteralSupport.h:75
+  // We use separate fields for fixed point sizes b/c the isHalf/isLong 
booleans
+  // assume that this literal is an integral type instead of fixed point type.
+  enum FixedPointSize { FPS_UNSPECIFIED, FPS_SHORT, FPS_LONG };

Shouldn't the flags be amended instead?



Comment at: lib/AST/ASTContext.cpp:1788
 case BuiltinType::UShortAccum:
+case BuiltinType::ShortFract:
+case BuiltinType::UShortFract:

See my comments on the _Accum patch.



Comment at: lib/AST/ASTDumper.cpp:2186
+  ColorScope Color(*this, ValueColor);
+  OS << " " << Node->getValue().toString(10, isSigned);
+}

This will not print as a fixed-point number.



Comment at: lib/AST/Expr.cpp:766
+  const auto *BT = type->getAs();
+  assert(BT && "Not a fixed point type!");
+  switch (BT->getKind()) {

Is this possible given the assertion above?



Comment at: lib/AST/Expr.cpp:767
+  assert(BT && "Not a fixed point type!");
+  switch (BT->getKind()) {
+default:

There is no reason to have this switch.



Comment at: lib/AST/Expr.cpp:774
+case BuiltinType::UShortFract:
+  assert(V.getBitWidth() == C.getIntWidth(type) &&
+ "Short fixed point type is not the correct size for constant.");

'getIntWidth' is likely the wrong accessor to use here. Fixed-point types are 
technically not ints.



Comment at: lib/AST/ExprConstant.cpp:9323
+  if (Value.isSigned() && Value.isMinSignedValue() && E->canOverflow() &&
+  !HandleOverflow(Info, E, -Value.extend(Value.getBitWidth() + 1),
+  E->getType()))

Is signed fixed-point overflow actually considered undefined behavior? I'm not 
familiar with what Embedded-C says about this.

Also, this routine will likely print the wrong number for fixed-point values.



Comment at: lib/AST/ExprConstant.cpp:9328
+}
+case UO_Not: {
+  if (!Visit(E->getSubExpr())) return false;

I do not believe ~ is valid on fixed-point types.



Comment at: lib/AST/StmtPrinter.cpp:1532
+  bool isSigned = Node->getType()->isSignedFixedPointType();
+  OS << Node->getValue().toString(10, isSigned);
+

This will not print a fixed-point number.



Comment at: lib/CodeGen/CGExprAgg.cpp:675
+  case CK_IntegralToFixedPoint:
+llvm_unreachable(
+"AggExprEmitter::VisitCastExpr CK_IntegralToFixedPoint");  // TODO

This probably goes in the large default case below, as fixed-point types are 
not aggregates.



Comment at: lib/CodeGen/CGExprComplex.cpp:451
+llvm_unreachable(
+"ComplexExprEmitter::EmitCast CK_IntegralToFixedPoint");  // TODO
   case CK_Dependent: llvm_unreachable("dependent cast kind in IR gen!");

Same as the aggregate case.



Comment at: lib/CodeGen/CGExprScalar.cpp:1789
+  case BuiltinType::ShortAccum:
+fbits = B

[PATCH] D46917: [Fixed Point Arithmetic] Comparison and Unary Operations for Fixed Point Types

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: lib/CodeGen/CGExprScalar.cpp:2141
+  case BuiltinType::ShortAccum:
+fbits = BUILTIN_SACCUM_FBIT;
+break;

Please see my comments on other patches about these values.



Comment at: lib/CodeGen/CGExprScalar.cpp:2178
+llvm::Value *amt =
+llvm::ConstantInt::get(value->getType(), 1 << fbits,
+   /*isSigned=*/type->isSignedFixedPointType());

Use an APInt with the right size here instead of relying on types in the 
compiler.


Repository:
  rC Clang

https://reviews.llvm.org/D46917



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46917: [Fixed Point Arithmetic] Comparison and Unary Operations for Fixed Point Types

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: lib/CodeGen/CGExprScalar.cpp:2178
+llvm::Value *amt =
+llvm::ConstantInt::get(value->getType(), 1 << fbits,
+   /*isSigned=*/type->isSignedFixedPointType());

ebevhan wrote:
> Use an APInt with the right size here instead of relying on types in the 
> compiler.
Also, what about saturating types?


Repository:
  rC Clang

https://reviews.llvm.org/D46917



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46927: [Fixed Point Arithmetic] Augmented Assignment for Fixed Point Types

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: include/clang/AST/Type.h:6577
+
+inline unsigned getFixedPointFBits(const QualType &Ty) {
+  return getFixedPointFBits(*Ty);

As I mentioned in another patch, this should be in ASTContext and ask 
TargetInfo for the information.

I'm not a fan of the term 'fbits'. The correct terminology should be the 
'scaling factor'.



Comment at: lib/CodeGen/CGExprScalar.cpp:1009
+  // a float and perform some floating point arithmetic, though this may cost
+  // more. Enable that if #pragma FX_FULL_PRECISION is provided.
+  if (dest_fbits > src_fbits) {

For future reference, please do not rely on the behavior of floating point for 
fixed-point operations.



Comment at: lib/CodeGen/CGExprScalar.cpp:1184
+// Casting down, so we will need to shift early as to not lose data
+Src = EmitFixedPointRadixShift(Src, SrcType, DstType);
+  }

I think it's safer to simply do all relevant fixed-point conversion by hand 
here instead of falling through and relying on the semantics of other 
conversions.

We can provide patches for our implementation here if wanted.



Comment at: lib/CodeGen/CGExprScalar.cpp:1214
 
+  if (WorkingOnFixedPoints && order >= 0) {
+// Casting up (or same type), so we can safely shift without losing data

This case should not be needed. Perform all fixed-point conversion before the 
integer type check above and do an early return instead.

It's not safe to rely on the behavior of other conversions for the behavior of 
these.


Repository:
  rC Clang

https://reviews.llvm.org/D46927



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46986: [Fixed Point Arithmetic] Validation Test for Fixed Point Binary Operations and Saturated Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

I cannot say that I'm pleased with the CodeGen emission of the operations as 
pure IR. I can only assume that you do not have hardware specifically tailored 
for these operations, as matching this type of code ought to be quite difficult 
after optimization is performed.




Comment at: include/clang/AST/Type.h:6591
+// saturated, return back the type itself.
+QualType getCorrespondingSaturatedFixedPointType(ASTContext &Context,
+ const Type &Ty);

If these need to be passed a ASTContext anyway, why not have these functions on 
ASTContext to begin with?



Comment at: include/clang/Basic/FixedPoint.h.in:35
+// Max values of each _Accum type as integer bytes
+#define SACCUM_MAX_AS_INT   ((1ULL << (BUILTIN_SACCUM_FBIT + 
BUILTIN_SACCUM_IBIT)) - 1)
+#define ACCUM_MAX_AS_INT((1ULL << (BUILTIN_ACCUM_FBIT + 
BUILTIN_ACCUM_IBIT)) - 1)

As mentioned in other patches, these should not be macros (this entire file 
should probably be removed altogether).



Comment at: lib/CodeGen/CGExprScalar.cpp:3151
+const auto &BT = op.Ty->getAs();
+switch (BT->getKind()) {
+  default:

All of these values can clearly be calculated based on the scaling factor and 
the width of the type.



Comment at: lib/CodeGen/CGExprScalar.cpp:3206
+if (op.Ty->isSignedFixedPointType()) {
+  MSBBitShift = getFixedPointIBits(op.Ty) + getFixedPointFBits(op.Ty);
+} else {

Factor out conversion between fixed-point types into its own function so it can 
be reused for other cases, such as for other operations and actual conversions. 
It should probably not take QualTypes to convert to but rather arbitrary widths 
and scales, so it can be used to upscale to/downscale from 'intermediate' 
common calculation types.



Comment at: lib/CodeGen/CGExprScalar.cpp:3211
+
+llvm::Value *Sum = Builder.CreateAdd(op.LHS, op.RHS);
+llvm::Value *LHSMSB = Builder.CreateLShr(op.LHS, MSBBitShift);

I would much rather see these operations emitted as intrinsics rather than 
straight IR... but I know that wasn't part of your proposal.



Comment at: lib/Sema/SemaExpr.cpp:1264
+/// integers or other fixed point types due to potential loss of precision.
+/// For this case of fixed point types, the resulting type in a binary 
operation
+/// does not need to be exactly one of the 2 operand types.

This is incorrect. The resulting type of a binary operation is absolutely one 
of the two operands. However, the calculation might be done as a type that is 
not one of the two, as it must be done in the full (combined) precision of both 
operands.

That is a detail reserved for CodeGen, however.



Comment at: lib/Sema/SemaExpr.cpp:3551
 uint64_t int_part_as_int = static_cast(int_part);
+uint64_t fract_part_as_int =
+static_cast(fract_part * (1ULL << fbits));

Should these changes not be part of the patch that adds the literal parsing 
code?


Repository:
  rC Clang

https://reviews.llvm.org/D46986



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46986: [Fixed Point Arithmetic] Validation Test for Fixed Point Binary Operations and Saturated Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

Also, this patch and all of the following 'Validation Test' patches do much 
more than just add tests, they add plenty of functionality as well. In general, 
it's quite difficult to tell which patches add what.

I think it would be much clearer if the patches that claim to add tests only 
add tests.


Repository:
  rC Clang

https://reviews.llvm.org/D46986



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:89
 SymMgr(context, BasicVals, alloc), MemMgr(context, alloc),
-StateMgr(stateMgr), ArrayIndexTy(context.LongLongTy),
+StateMgr(stateMgr), ArrayIndexTy(context.getSignedSizeType()),
 ArrayIndexWidth(context.getTypeSize(ArrayIndexTy)) {}

a.sidorin wrote:
> As you correctly pointed, ssize_t is 32-bit on 32-bit systems. Therefore, it 
> is too short. So, we can leave this line as-is.
But if it's hardcoded to LongLongTy, you have the same problem on 64-bit 
systems.


https://reviews.llvm.org/D46944



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

[Bevin Hansson via Phabricator via cfe-commits]

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 correct to 
consider 'short' the 'underlying type' of 'short _Accum', 'int' the 'underlying 
type' of '_Accum', etc. They are wholly independent and should have separate 
settings altogether.

Asserting/ensuring that a target has set an invalid width for its types should 
be done separately. This currently feels a bit like the TargetInfo is guessing.

(For the record, the reason I'm requesting this change is because this 
implementation does not work for us downstream.)


Repository:
  rC Clang

https://reviews.llvm.org/D46084



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

[Bevin Hansson via Phabricator via cfe-commits]

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 superfluous.



Comment at: include/clang/Driver/Options.td:882
 
+def enable_fixed_point : Flag<["-", "--"], "enable-fixed-point">, 
Group,
+ Flags<[CC1Option]>, HelpText<"Enable fixed point 
types">;

Shouldn't this be an `-f` flag, like `-ffixed-point`? I'm not for or against 
either, just wondering what anyone else thinks.


Repository:
  rC Clang

https://reviews.llvm.org/D46084



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h:89
 SymMgr(context, BasicVals, alloc), MemMgr(context, alloc),
-StateMgr(stateMgr), ArrayIndexTy(context.LongLongTy),
+StateMgr(stateMgr), ArrayIndexTy(context.getSignedSizeType()),
 ArrayIndexWidth(context.getTypeSize(ArrayIndexTy)) {}

a.sidorin wrote:
> ebevhan wrote:
> > a.sidorin wrote:
> > > As you correctly pointed, ssize_t is 32-bit on 32-bit systems. Therefore, 
> > > it is too short. So, we can leave this line as-is.
> > But if it's hardcoded to LongLongTy, you have the same problem on 64-bit 
> > systems.
> Some reasons why LongLongTy is used here are listed in D16063. In brief, you 
> just cannot create an array of size greater than SIZE_MAX/2  on 64-bit 
> platforms.
I don't think that's limited to 64-bit platforms, it applies to 32-bit ones as 
well. I know that LLVM has issues with indexing arrays that are larger than 
half of the address space in general due to limitations of GEP.


https://reviews.llvm.org/D46944



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan created this revision.
ebevhan added reviewers: krememek, rsmith.
Herald added a subscriber: cfe-commits.

The integer promotions apply to bitfields as well, but
rather oddly. If you have a bitfield with a lower width
than int, then the type of the member expression will
be int regardless of the type of the bitfield member.
This means that you can effectively get 'type demotion'
in a bitfield member expression.

However, when analyzing format string types, we
would look through argument promotions like integer
promotion. Looking through bitfield demotion means
that we would get the wrong type when analyzing,
hiding -Wformat issues.

This patch fixes this so that we only explicitly look
through integer and floating point promotion where
the result type is actually a promotion.


Repository:
  rC Clang

https://reviews.llvm.org/D51211

Files:
  lib/Sema/SemaChecking.cpp
  test/Sema/format-strings.c


Index: test/Sema/format-strings.c
===
--- test/Sema/format-strings.c
+++ test/Sema/format-strings.c
@@ -692,3 +692,17 @@
   // This caused crashes due to invalid casts.
   printf(1 > 0); // expected-warning{{format string is not a string literal}} 
expected-warning{{incompatible integer to pointer conversion}} 
expected-note@format-strings.c:*{{passing argument to parameter here}} 
expected-note{{to avoid this}}
 }
+
+struct bitfields {
+  long a : 2;
+  unsigned long b : 2;
+  long c : 32;  // assumes that int is 32 bits
+  unsigned long d : 32; // assumes that int is 32 bits
+} bf;
+
+void bitfield_promotion() {
+  printf("%ld", bf.a); // expected-warning {{format specifies type 'long' but 
the argument has type 'int'}}
+  printf("%lu", bf.b); // expected-warning {{format specifies type 'unsigned 
long' but the argument has type 'int'}}
+  printf("%ld", bf.c); // expected-warning {{format specifies type 'long' but 
the argument has type 'int'}}
+  printf("%lu", bf.d); // expected-warning {{format specifies type 'unsigned 
long' but the argument has type 'unsigned int'}}
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -7582,6 +7582,26 @@
   return std::make_pair(QualType(), StringRef());
 }
 
+/// Return true if \p ICE is an implicit argument promotion of an arithmetic
+/// type. Bit-field 'promotions' from a higher ranked type to a lower ranked
+/// type do not count.
+static bool
+isArithmeticArgumentPromotion(Sema &S, const ImplicitCastExpr *ICE) {
+  QualType From = ICE->getSubExpr()->getType();
+  QualType To = ICE->getType();
+  // It's a floating promotion if the source type is a lower rank.
+  if (ICE->getCastKind() == CK_FloatingCast &&
+  S.Context.getFloatingTypeOrder(From, To) < 0)
+return true;
+  // It's an integer promotion if the destination type is the promoted
+  // source type.
+  if (ICE->getCastKind() == CK_IntegralCast &&
+  From->isPromotableIntegerType() &&
+  S.Context.getPromotedIntegerType(From) == To)
+return true;
+  return false;
+}
+
 bool
 CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
 const char *StartSpecifier,
@@ -7609,11 +7629,11 @@
 
   // Look through argument promotions for our error message's reported type.
   // This includes the integral and floating promotions, but excludes array
-  // and function pointer decay; seeing that an argument intended to be a
-  // string has type 'char [6]' is probably more confusing than 'char *'.
+  // and function pointer decay (seeing that an argument intended to be a
+  // string has type 'char [6]' is probably more confusing than 'char *') and
+  // certain bitfield promotions (bitfields can be 'demoted' to a lesser type).
   if (const ImplicitCastExpr *ICE = dyn_cast(E)) {
-if (ICE->getCastKind() == CK_IntegralCast ||
-ICE->getCastKind() == CK_FloatingCast) {
+if (isArithmeticArgumentPromotion(S, ICE)) {
   E = ICE->getSubExpr();
   ExprTy = E->getType();
 


Index: test/Sema/format-strings.c
===
--- test/Sema/format-strings.c
+++ test/Sema/format-strings.c
@@ -692,3 +692,17 @@
   // This caused crashes due to invalid casts.
   printf(1 > 0); // expected-warning{{format string is not a string literal}} expected-warning{{incompatible integer to pointer conversion}} expected-note@format-strings.c:*{{passing argument to parameter here}} expected-note{{to avoid this}}
 }
+
+struct bitfields {
+  long a : 2;
+  unsigned long b : 2;
+  long c : 32;  // assumes that int is 32 bits
+  unsigned long d : 32; // assumes that int is 32 bits
+} bf;
+
+void bitfield_promotion() {
+  printf("%ld", bf.a); // expected-warning {{format specifies type 'long' but the argument has type 'int'}}
+  printf("%lu", bf.b); // expected-warning {{format specifies type 'unsigned long' b

[PATCH] D51426: [Sema] Prohibit function-scope compound literals with address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan created this revision.
ebevhan added reviewers: efriedma, rjmccall.
Herald added a subscriber: cfe-commits.

In C, compound literals in function scope are lvalues with
automatic storage duration. This means that generally, they
cannot be address space-qualified since you cannot have
address space-qualified objects with automatic storage duration.

We might want to check the 'alloca address space' here, but
neither ASTContext nor TargetInfo seem to have this.


Repository:
  rC Clang

https://reviews.llvm.org/D51426

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaExpr.cpp
  test/Sema/address_spaces.c


Index: test/Sema/address_spaces.c
===
--- test/Sema/address_spaces.c
+++ test/Sema/address_spaces.c
@@ -73,3 +73,17 @@
 char* cmp(_AS1 char *x,  _AS2 char *y) {
   return x < y ? x : y; // expected-error{{conditional operator with the 
second and third operands of type  ('__attribute__((address_space(1))) char *' 
and '__attribute__((address_space(2))) char *') which are pointers to 
non-overlapping address spaces}}
 }
+
+struct SomeStruct {
+  int a;
+  long b;
+  int c;
+};
+
+// Compound literals in function scope are lvalues with automatic storage 
duration,
+// so they cannot realistically be qualified with an address space.
+void as_compound_literal() {
+  (_AS1 struct SomeStruct){1, 2, 3}; // expected-error {{compound literal in 
function scope may not be qualified with an address space}}
+  (_AS1 char[]){"test"}; // expected-error {{compound literal in function 
scope may not be qualified with an address space}}
+  (_AS1 char[]){'a', 'b', 'c'}; // expected-error {{compound literal in 
function scope may not be qualified with an address space}}
+}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -5733,12 +5733,19 @@
   LiteralExpr = Result.get();
 
   bool isFileScope = !CurContext->isFunctionOrMethod();
-  if (isFileScope &&
-  !LiteralExpr->isTypeDependent() &&
-  !LiteralExpr->isValueDependent() &&
-  !literalType->isDependentType()) { // 6.5.2.5p3
-if (CheckForConstantInitializer(LiteralExpr, literalType))
-  return ExprError();
+  if (isFileScope) {
+if(!LiteralExpr->isTypeDependent() &&
+   !LiteralExpr->isValueDependent() &&
+   !literalType->isDependentType()) // C99 6.5.2.5p3
+  if (CheckForConstantInitializer(LiteralExpr, literalType))
+return ExprError();
+  } else if(literalType.getAddressSpace() != LangAS::opencl_private &&
+literalType.getAddressSpace() != LangAS::Default) {
+// C99 6.5.2.5p6: Function scope compound literals must have automatic
+// storage which generally excludes address space-qualified ones.
+Diag(LParenLoc, diag::err_compound_literal_with_address_space)
+  << SourceRange(LParenLoc, LiteralExpr->getSourceRange().getEnd());
+return ExprError();
   }
 
   // In C, compound literals are l-values for some reason.
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -2603,6 +2603,8 @@
   "parameter may not be qualified with an address space">;
 def err_field_with_address_space : Error<
   "field may not be qualified with an address space">;
+def err_compound_literal_with_address_space : Error<
+  "compound literal in function scope may not be qualified with an address 
space">;
 def err_attr_objc_ownership_redundant : Error<
   "the type %0 is already explicitly ownership-qualified">;
 def err_invalid_nsnumber_type : Error<


Index: test/Sema/address_spaces.c
===
--- test/Sema/address_spaces.c
+++ test/Sema/address_spaces.c
@@ -73,3 +73,17 @@
 char* cmp(_AS1 char *x,  _AS2 char *y) {
   return x < y ? x : y; // expected-error{{conditional operator with the second and third operands of type  ('__attribute__((address_space(1))) char *' and '__attribute__((address_space(2))) char *') which are pointers to non-overlapping address spaces}}
 }
+
+struct SomeStruct {
+  int a;
+  long b;
+  int c;
+};
+
+// Compound literals in function scope are lvalues with automatic storage duration,
+// so they cannot realistically be qualified with an address space.
+void as_compound_literal() {
+  (_AS1 struct SomeStruct){1, 2, 3}; // expected-error {{compound literal in function scope may not be qualified with an address space}}
+  (_AS1 char[]){"test"}; // expected-error {{compound literal in function scope may not be qualified with an address space}}
+  (_AS1 char[]){'a', 'b', 'c'}; // expected-error {{compound literal in function scope may not be qualified with an address space}}
+}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.

[PATCH] D51426: [Sema] Prohibit function-scope compound literals with address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:5745
+// C99 6.5.2.5p6: Function scope compound literals must have automatic
+// storage which generally excludes address space-qualified ones.
+Diag(LParenLoc, diag::err_compound_literal_with_address_space)

rjmccall wrote:
> Usually when we mention a standard section like this, it's a prelude to a 
> quote.  If you're just paraphrasing, I think we can trust people to find the 
> right standard section.
Hm, alright. I figured it was better to both provide the exact section and also 
include a summary here so you don't have to look it up.

Should I change it or is it good anyway?


Repository:
  rC Clang

https://reviews.llvm.org/D51426



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51426: [Sema] Prohibit function-scope compound literals with address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan updated this revision to Diff 163277.
ebevhan edited the summary of this revision.
ebevhan added a comment.

Added Embedded-C clause quote.
Fixed formatting.


https://reviews.llvm.org/D51426

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaExpr.cpp
  test/Sema/address_spaces.c


Index: test/Sema/address_spaces.c
===
--- test/Sema/address_spaces.c
+++ test/Sema/address_spaces.c
@@ -73,3 +73,17 @@
 char* cmp(_AS1 char *x,  _AS2 char *y) {
   return x < y ? x : y; // expected-error{{conditional operator with the 
second and third operands of type  ('__attribute__((address_space(1))) char *' 
and '__attribute__((address_space(2))) char *') which are pointers to 
non-overlapping address spaces}}
 }
+
+struct SomeStruct {
+  int a;
+  long b;
+  int c;
+};
+
+// Compound literals in function scope are lvalues with automatic storage 
duration,
+// so they cannot realistically be qualified with an address space.
+void as_compound_literal() {
+  (_AS1 struct SomeStruct){1, 2, 3}; // expected-error {{compound literal in 
function scope may not be qualified with an address space}}
+  (_AS1 char[]){"test"}; // expected-error {{compound literal in function 
scope may not be qualified with an address space}}
+  (_AS1 char[]){'a', 'b', 'c'}; // expected-error {{compound literal in 
function scope may not be qualified with an address space}}
+}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -5733,12 +5733,20 @@
   LiteralExpr = Result.get();
 
   bool isFileScope = !CurContext->isFunctionOrMethod();
-  if (isFileScope &&
-  !LiteralExpr->isTypeDependent() &&
-  !LiteralExpr->isValueDependent() &&
-  !literalType->isDependentType()) { // 6.5.2.5p3
-if (CheckForConstantInitializer(LiteralExpr, literalType))
-  return ExprError();
+  if (isFileScope) {
+if (!LiteralExpr->isTypeDependent() &&
+!LiteralExpr->isValueDependent() &&
+!literalType->isDependentType()) // C99 6.5.2.5p3
+  if (CheckForConstantInitializer(LiteralExpr, literalType))
+return ExprError();
+  } else if (literalType.getAddressSpace() != LangAS::opencl_private &&
+ literalType.getAddressSpace() != LangAS::Default) {
+// Embedded-C extensions to C99 6.5.2.5:
+//   "If the compound literal occurs inside the body of a function, the
+//   type name shall not be qualified by an address-space qualifier."
+Diag(LParenLoc, diag::err_compound_literal_with_address_space)
+  << SourceRange(LParenLoc, LiteralExpr->getSourceRange().getEnd());
+return ExprError();
   }
 
   // In C, compound literals are l-values for some reason.
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -2603,6 +2603,8 @@
   "parameter may not be qualified with an address space">;
 def err_field_with_address_space : Error<
   "field may not be qualified with an address space">;
+def err_compound_literal_with_address_space : Error<
+  "compound literal in function scope may not be qualified with an address 
space">;
 def err_attr_objc_ownership_redundant : Error<
   "the type %0 is already explicitly ownership-qualified">;
 def err_invalid_nsnumber_type : Error<


Index: test/Sema/address_spaces.c
===
--- test/Sema/address_spaces.c
+++ test/Sema/address_spaces.c
@@ -73,3 +73,17 @@
 char* cmp(_AS1 char *x,  _AS2 char *y) {
   return x < y ? x : y; // expected-error{{conditional operator with the second and third operands of type  ('__attribute__((address_space(1))) char *' and '__attribute__((address_space(2))) char *') which are pointers to non-overlapping address spaces}}
 }
+
+struct SomeStruct {
+  int a;
+  long b;
+  int c;
+};
+
+// Compound literals in function scope are lvalues with automatic storage duration,
+// so they cannot realistically be qualified with an address space.
+void as_compound_literal() {
+  (_AS1 struct SomeStruct){1, 2, 3}; // expected-error {{compound literal in function scope may not be qualified with an address space}}
+  (_AS1 char[]){"test"}; // expected-error {{compound literal in function scope may not be qualified with an address space}}
+  (_AS1 char[]){'a', 'b', 'c'}; // expected-error {{compound literal in function scope may not be qualified with an address space}}
+}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -5733,12 +5733,20 @@
   LiteralExpr = Result.get();
 
   bool isFileScope = !CurContext->isFunctionOrMethod();
-  if (isFileScope &&
-  !LiteralExpr->isTypeDependent() &&
-  !LiteralExpr->isValueDependent() &&
-  !literalType->i

[PATCH] D51426: [Sema] Prohibit function-scope compound literals with address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

Thanks!

I don't have commit access, so it would be appreciated if someone could commit 
it.


https://reviews.llvm.org/D51426



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

Ping.


Repository:
  rC Clang

https://reviews.llvm.org/D51211



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

Sorry for the late response, I've been away.

LGTM, although my browser doesn't want to let me set Accepted on this.




Comment at: unittests/Basic/FixedPointTest.cpp:380
+}
+
+// Check the value in a given fixed point sema overflows to the saturated max

leonardchan wrote:
> ebevhan wrote:
> > Technically, neither CheckSaturatedConversion function checks whether or 
> > not the result was correct, only that it saturated.
> Could you elaborate on this? It should be correct that the value saturates to 
> the respective min/max for the destination type right?
It should... I'm not sure what I was thinking of when I wrote this comment, and 
I can't think of a counterexample so it's fine as it is.



Comment at: unittests/Basic/FixedPointTest.cpp:506
+TEST(FixedPoint, AccConversionOverflow) {
+  // To SAcc max limit (65536)
+  CheckSaturatedConversionMax(getLAccSema(), Saturated(getAccSema()),

leonardchan wrote:
> ebevhan wrote:
> > Acc?
> Short for Accum. Changed to `Accum` since it wasn't clear at first.
Seems like you got a couple `Accumum` from that.


Repository:
  rC Clang

https://reviews.llvm.org/D48661



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49945: [Fixed Point Arithmetic] Fix for FixedPointValueToString

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan accepted this revision.
ebevhan added a comment.
This revision is now accepted and ready to land.

LGTM.


Repository:
  rC Clang

https://reviews.llvm.org/D49945



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50278: [Sema] Fix for crash on conditional operation with address_space pointer

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

When I try the test case on our downstream (and when compiling for our target 
with an extra flag that enables a bunch of OpenCL-related address space code), 
I get:

  ascrash.c:5:12: error: comparison between ('__attribute__((address_space(1))) 
char *' and '__attribute__((address_space(2))) char *') which
are pointers to non-overlapping address spaces
return x < y ? x : y;
   ~ ^ ~
  ascrash.c:5:16: error: conditional operator with the second and third 
operands of type ('__attribute__((address_space(1))) char *' and
'__attribute__((address_space(2))) char *') which are pointers to 
non-overlapping address spaces
return x < y ? x : y;
 ^ ~   ~

A lot of address space code is hidden behind LangOptions.OpenCL.




Comment at: lib/Sema/SemaExpr.cpp:6464
   LangAS RAddrSpace = rhQual.getAddressSpace();
   if (S.getLangOpts().OpenCL) {
 // OpenCL v1.1 s6.5 - Conversion between pointers to distinct address

Here is an OpenCL condition.



Comment at: lib/Sema/SemaExpr.cpp:6473
   S.Diag(Loc,
  diag::err_typecheck_op_on_nonoverlapping_address_space_pointers)
   << LHSTy << RHSTy << 2 << LHS.get()->getSourceRange()

Aren't these the errors you actually want?



Comment at: lib/Sema/SemaExpr.cpp:8860
   // if both are pointers check if operation is valid wrt address spaces
   if (S.getLangOpts().OpenCL && isLHSPointer && isRHSPointer) {
 const PointerType *lhsPtr = LHSExpr->getType()->getAs();

Here is another OpenCL AS error.



Comment at: lib/Sema/SemaExpr.cpp:10204
   // Treat NULL constant as a special case in OpenCL.
   if (getLangOpts().OpenCL && !LHSIsNull && !RHSIsNull) {
 const PointerType *LHSPtr = LHSType->getAs();

And another.


Repository:
  rC Clang

https://reviews.llvm.org/D50278



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49945: [Fixed Point Arithmetic] Fix for FixedPointValueToString

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: lib/AST/Expr.cpp:788
   FixedPointValueToString(
-  S, llvm::APSInt::getUnsigned(getValue().getZExtValue()), Scale, Radix);
+  S, llvm::APSInt::getUnsigned(getValue().getZExtValue()), Scale);
   return S.str();

Unrelated to this patch specifically, but using `getZExtValue` here is a bit 
limiting.


Repository:
  rC Clang

https://reviews.llvm.org/D49945



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

[Bevin Hansson via Phabricator via cfe-commits]

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 keyword even if -fno-fixed-point 
is given. Is this desired? It will mean that `_Accum` will not be a valid 
identifier in standard C regardless of the flag.


Repository:
  rC Clang

https://reviews.llvm.org/D46084



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

[Bevin Hansson via Phabricator via cfe-commits]

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 for the 
> > non-Accum versions will have .e.g. `sizeof(short) != sizeof(short _Accum)`. 
> > Is there a point in ever having `_Accum` differ in size, width, and 
> > alignment from the underlying type? If not, can you set these values after 
> > the sub-target has specified its preferences?
> I'm uncomfortable about opting all targets into this behavior with these 
> default values; this will result in an ABI break if later a target updates 
> these to the correct values. A per-target `AccumSupported` flag would help 
> substantially. But this is OK for the short term while you're still working 
> on the feature.
> 
> We'll also need the target to inform us of the number of integer and 
> fractional bits for each such type.
> We'll also need the target to inform us of the number of integer and 
> fractional bits for each such type.

I believe the only one that is needed is for the number of fractional bits; the 
number of integer bits is implied by the difference between the type width and 
fractional bits. I think I mention this in one of the other patches.




Repository:
  rC Clang

https://reviews.llvm.org/D46084



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46911: [Fixed Point Arithmetic] Addition of the remaining fixed point types and their saturated equivalents

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: include/clang/AST/Type.h:6551
+
+QualType getCorrespondingSaturatedType(const ASTContext &Context,
+   const QualType &Ty);

These should probably be in ASTContext directly.



Comment at: include/clang/Basic/TargetInfo.h:83
+  unsigned char LongFractWidth, LongFractAlign;
+  unsigned char SatShortAccumWidth, SatShortAccumAlign;
+  unsigned char SatAccumWidth, SatAccumAlign;

I don't think the saturating types need separate configurations. Embedded-C 
says "Each saturating fixed-point type has the same representation and the same 
rank as its corresponding primary fixed-point type."



Comment at: lib/Parse/ParseDecl.cpp:3614
+  } else {
+isInvalid = DS.SetTypeSpecSat(/*isSat=*/true, Loc, PrevSpec, DiagID);
+  }

Is there a use for the isSat parameter?



Comment at: lib/Sema/DeclSpec.cpp:1123
+if (!(TypeSpecType == TST_accum || TypeSpecType == TST_fract)) {
+  S.Diag(TSSatLoc, diag::err_invalid_saturation_spec)
+  << getSpecifierName((TST)TypeSpecType, Policy);

Handling this case here means that placing _Sat on something other than exactly 
a fixed-point type is a parsing error rather than a semantic error. How does 
this handle _Sat on sugared types? Should _Sat on things like typedefs work?

  typedef _Fract myfract;
  _Sat myfract F;

The primary issue (and this is one that we have encountered as well) is that 
you cannot have a true _Sat typedef since _Sat only exists as part of builtin 
types. You need to desugar/canonicalize the type and then do 
getCorrespondingSaturatingType (or have getCorrespondingSaturatingType look at 
the canonical type internally).



Comment at: lib/Sema/DeclSpec.cpp:1135
  TypeSpecType != TST_char && TypeSpecType != TST_wchar &&
- TypeSpecType != TST_accum) {
+ TypeSpecType != TST_accum && TypeSpecType != TST_fract) {
   S.Diag(TSSLoc, diag::err_invalid_sign_spec)

IsFixedPointType can be used here as well.



Comment at: lib/Sema/DeclSpec.cpp:1165
 else if (TypeSpecType != TST_int && TypeSpecType != TST_double &&
- TypeSpecType != TST_accum) {
+ TypeSpecType != TST_accum && TypeSpecType != TST_fract) {
   S.Diag(TSWRange.getBegin(), diag::err_invalid_width_spec)

IsFixedPointType?



Comment at: lib/Sema/SemaType.cpp:1410
+
+if (DS.getTypeSpecSign() != DeclSpec::TSS_unsigned)
+  Result = fixedpoint::getCorrespondingSignedType(Context, Result);

The logic is a bit reversed. The default should be to select the signed 
variant, and if the TSS is unsigned, then convert it to the unsigned variant.



Comment at: lib/Sema/SemaType.cpp:1609
 declarator.setInvalidType(true);
 
   // Handle complex types.

Other qualifiers like const and volatile are handled down here. Should the _Sat 
application be performed somewhere here instead?


Repository:
  rC Clang

https://reviews.llvm.org/D46911



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

[Bevin Hansson via Phabricator via cfe-commits]

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:
> > > > leonardchan wrote:
> > > > > rsmith wrote:
> > > > > > jfb wrote:
> > > > > > > This seems weird because Targets which don't have these values 
> > > > > > > for the non-Accum versions will have .e.g. `sizeof(short) != 
> > > > > > > sizeof(short _Accum)`. Is there a point in ever having `_Accum` 
> > > > > > > differ in size, width, and alignment from the underlying type? If 
> > > > > > > not, can you set these values after the sub-target has specified 
> > > > > > > its preferences?
> > > > > > I'm uncomfortable about opting all targets into this behavior with 
> > > > > > these default values; this will result in an ABI break if later a 
> > > > > > target updates these to the correct values. A per-target 
> > > > > > `AccumSupported` flag would help substantially. But this is OK for 
> > > > > > the short term while you're still working on the feature.
> > > > > > 
> > > > > > We'll also need the target to inform us of the number of integer 
> > > > > > and fractional bits for each such type.
> > > > > The integral and fractional bits will be set in the target and is 
> > > > > available in a later patch.
> > > > > We'll also need the target to inform us of the number of integer and 
> > > > > fractional bits for each such type.
> > > > 
> > > > I believe the only one that is needed is for the number of fractional 
> > > > bits; the number of integer bits is implied by the difference between 
> > > > the type width and fractional bits. I think I mention this in one of 
> > > > the other patches.
> > > > 
> > > > 
> > > You're right. I was stuck in the mindset that we would be providing an 
> > > integral and fractional value.
> > > The integral and fractional bits will be set in the target and is 
> > > available in a later patch.
> > 
> > I mean just the fractional bits since the integral will just be the bit 
> > width minus fractional.
> You're assuming that all bits will be either integral or fractional bits (and 
> in particular that there are no padding bits). I don't know if that's 
> actually going to be true for all target ABIs, but I suppose it's OK to make 
> this assumption until it's proven wrong by some actual target.
It is actually the case for us (downstream) that the unsigned types should have 
one bit of padding, however, this is a special case. The spec says "Each 
unsigned fract type has either the same number of fractional bits as, or one 
more fractional bit than, its corresponding signed fract type." and also under 
'recommended practice', "Each signed accum type has the same number of 
fractional bits as either its corresponding signed fract type or its 
corresponding unsigned fract type."

So I think the approach would be that the integral bits are implied from the 
fractional bits and the width, except in the unsigned case where the MSB is a 
padding bit. If there is a target that really does want the unsigned types to 
have an extra bit of precision it could be added as a flag, but I don't really 
see the benefit of it. The consistency benefit of having the unsigned types 
have the same scaling factor as the signed ones outweighs the downsides.


Repository:
  rC Clang

https://reviews.llvm.org/D46084



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46911: [Fixed Point Arithmetic] Addition of the remaining fixed point types and their saturated equivalents

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: include/clang/AST/ASTContext.h:2882
+
+  QualType getCorrespondingSaturatedType(const QualType &Ty) const;
 };

This probably belongs up near the other predicates.

Also I think it's more common to simply pass `QualType` instead of a `const 
QualType&`.



Comment at: lib/Sema/DeclSpec.cpp:1123
+if (!(TypeSpecType == TST_accum || TypeSpecType == TST_fract)) {
+  S.Diag(TSSatLoc, diag::err_invalid_saturation_spec)
+  << getSpecifierName((TST)TypeSpecType, Policy);

leonardchan wrote:
> ebevhan wrote:
> > Handling this case here means that placing _Sat on something other than 
> > exactly a fixed-point type is a parsing error rather than a semantic error. 
> > How does this handle _Sat on sugared types? Should _Sat on things like 
> > typedefs work?
> > 
> >   typedef _Fract myfract;
> >   _Sat myfract F;
> > 
> > The primary issue (and this is one that we have encountered as well) is 
> > that you cannot have a true _Sat typedef since _Sat only exists as part of 
> > builtin types. You need to desugar/canonicalize the type and then do 
> > getCorrespondingSaturatingType (or have getCorrespondingSaturatingType look 
> > at the canonical type internally).
> I think _Sat is analogous to _Complex where it only works with specific 
> builtin types, albeit the only builtin type _Complex doesn't work with is 
> _Bool.
> 
> Currently this example would throw the error `'_Sat' specifier is only valid 
> on '_Fract' or '_Accum', not 'type-name'` which is similar to what _Complex 
> does when paired with a typedef:
> 
> ```
> typedef double mydouble;
> mydouble _Complex D;  // _Complex type-name' is invalid
> ```
> 
> I don't see this as a big problem right now, but am willing to come back to 
> this in the future if it becomes more urgent. For now, I added a test that 
> asserts this error is thrown.
That sounds reasonable. I have no strong opinions on it and I don't think we 
use the functionality anyway.



Comment at: lib/Sema/SemaType.cpp:1612
+  // Only fixed point types can be saturated
+  if (DS.isTypeSpecSat()) {
+if (!IsFixedPointType) {

The braces aren't needed.


Repository:
  rC Clang

https://reviews.llvm.org/D46911



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46944: [analyzer] Use sufficiently large types for index/size calculation.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan updated this revision to Diff 149415.
ebevhan edited the summary of this revision.
ebevhan added a comment.

Changed ArrayIndexTy back to LongLongTy and reverted the test change.


https://reviews.llvm.org/D46944

Files:
  lib/StaticAnalyzer/Core/ProgramState.cpp
  lib/StaticAnalyzer/Core/RegionStore.cpp
  test/Analysis/index-type.c


Index: test/Analysis/index-type.c
===
--- test/Analysis/index-type.c
+++ test/Analysis/index-type.c
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 
-analyzer-checker=core,alpha.security.ArrayBoundV2 -verify %s
-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 
-analyzer-checker=core,alpha.security.ArrayBoundV2 -DM32 -verify %s
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 
-analyzer-checker=core,alpha.security.ArrayBoundV2 
-Wno-implicit-function-declaration -verify %s
+// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 
-analyzer-checker=core,alpha.security.ArrayBoundV2 
-Wno-implicit-function-declaration -DM32 -verify %s
 // expected-no-diagnostics
 
 #define UINT_MAX (~0u)
@@ -36,4 +36,23 @@
   *ptr = 42; // no-warning
 }
 
+#define SIZE 4294967296
+
+static unsigned size;
+static void * addr;
+static unsigned buf[SIZE];
+
+void testOutOfBounds() {
+  // Not out of bounds.
+  buf[SIZE-1] = 1; // no-warning
+}
+
+void testOutOfBoundsCopy1() {
+  memcpy(buf, addr, size); // no-warning
+}
+
+void testOutOfBoundsCopy2() {
+  memcpy(addr, buf, size); // no-warning
+}
+
 #endif
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1341,7 +1341,8 @@
   // If a variable is reinterpreted as a type that doesn't fit into a larger
   // type evenly, round it down.
   // This is a signed value, since it's used in arithmetic with signed indices.
-  return svalBuilder.makeIntVal(RegionSize / EleSize, false);
+  return svalBuilder.makeIntVal(RegionSize / EleSize,
+svalBuilder.getArrayIndexType());
 }
 
 
//===--===//
Index: lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- lib/StaticAnalyzer/Core/ProgramState.cpp
+++ lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -336,9 +336,8 @@
 
   // Get the offset: the minimum value of the array index type.
   BasicValueFactory &BVF = svalBuilder.getBasicValueFactory();
-  // FIXME: This should be using ValueManager::ArrayindexTy...somehow.
   if (indexTy.isNull())
-indexTy = Ctx.IntTy;
+indexTy = svalBuilder.getArrayIndexType();
   nonloc::ConcreteInt Min(BVF.getMinValue(indexTy));
 
   // Adjust the index.


Index: test/Analysis/index-type.c
===
--- test/Analysis/index-type.c
+++ test/Analysis/index-type.c
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,alpha.security.ArrayBoundV2 -verify %s
-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -analyzer-checker=core,alpha.security.ArrayBoundV2 -DM32 -verify %s
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10 -analyzer-checker=core,alpha.security.ArrayBoundV2 -Wno-implicit-function-declaration -verify %s
+// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -analyzer-checker=core,alpha.security.ArrayBoundV2 -Wno-implicit-function-declaration -DM32 -verify %s
 // expected-no-diagnostics
 
 #define UINT_MAX (~0u)
@@ -36,4 +36,23 @@
   *ptr = 42; // no-warning
 }
 
+#define SIZE 4294967296
+
+static unsigned size;
+static void * addr;
+static unsigned buf[SIZE];
+
+void testOutOfBounds() {
+  // Not out of bounds.
+  buf[SIZE-1] = 1; // no-warning
+}
+
+void testOutOfBoundsCopy1() {
+  memcpy(buf, addr, size); // no-warning
+}
+
+void testOutOfBoundsCopy2() {
+  memcpy(addr, buf, size); // no-warning
+}
+
 #endif
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1341,7 +1341,8 @@
   // If a variable is reinterpreted as a type that doesn't fit into a larger
   // type evenly, round it down.
   // This is a signed value, since it's used in arithmetic with signed indices.
-  return svalBuilder.makeIntVal(RegionSize / EleSize, false);
+  return svalBuilder.makeIntVal(RegionSize / EleSize,
+svalBuilder.getArrayIndexType());
 }
 
 //===--===//
Index: lib/StaticAnalyzer/Core/ProgramState.cpp
===
--- lib/StaticAnalyzer/Core/ProgramState.cpp
+++ lib/StaticAnalyzer/Core/ProgramState.cpp
@@ -336,9 +336,8 @@
 
   // Get the offset: the minim

[PATCH] D47627: [ASTContext] Make getAddrSpaceQualType replace address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan created this revision.
ebevhan added reviewers: rjmccall, arichardson.
Herald added a subscriber: cfe-commits.

The documentation for getAddrSpaceQualType says: "If T already
has an address space specifier, it is silently replaced."

The function did not do this; it asserted instead. Fix it so
that it behaves according to the comment.


Repository:
  rC Clang

https://reviews.llvm.org/D47627

Files:
  lib/AST/ASTContext.cpp


Index: lib/AST/ASTContext.cpp
===
--- lib/AST/ASTContext.cpp
+++ lib/AST/ASTContext.cpp
@@ -2496,12 +2496,16 @@
   const Type *TypeNode = Quals.strip(T);
 
   // If this type already has an address space specified, it cannot get
-  // another one.
-  assert(!Quals.hasAddressSpace() &&
- "Type cannot be in multiple addr spaces!");
-  Quals.addAddressSpace(AddressSpace);
+  // another one. Replace it.
+  if (Quals.hasAddressSpace())
+Quals.removeAddressSpace();
+  if (AddressSpace != LangAS::Default)
+Quals.addAddressSpace(AddressSpace);
 
-  return getExtQualType(TypeNode, Quals);
+  if (Quals.hasNonFastQualifiers())
+return getExtQualType(TypeNode, Quals);
+  else
+return QualType(TypeNode, Quals.getFastQualifiers());
 }
 
 QualType ASTContext::removeAddrSpaceQualType(QualType T) const {


Index: lib/AST/ASTContext.cpp
===
--- lib/AST/ASTContext.cpp
+++ lib/AST/ASTContext.cpp
@@ -2496,12 +2496,16 @@
   const Type *TypeNode = Quals.strip(T);
 
   // If this type already has an address space specified, it cannot get
-  // another one.
-  assert(!Quals.hasAddressSpace() &&
- "Type cannot be in multiple addr spaces!");
-  Quals.addAddressSpace(AddressSpace);
+  // another one. Replace it.
+  if (Quals.hasAddressSpace())
+Quals.removeAddressSpace();
+  if (AddressSpace != LangAS::Default)
+Quals.addAddressSpace(AddressSpace);
 
-  return getExtQualType(TypeNode, Quals);
+  if (Quals.hasNonFastQualifiers())
+return getExtQualType(TypeNode, Quals);
+  else
+return QualType(TypeNode, Quals.getFastQualifiers());
 }
 
 QualType ASTContext::removeAddrSpaceQualType(QualType T) const {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47630: [Sema] Allow creating types with multiple of the same addrspace.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan created this revision.
ebevhan added a reviewer: Anastasia.
Herald added a subscriber: cfe-commits.

The comment with the OpenCL clause about this clearly
says: "No type shall be qualified by qualifiers for
two or more different address spaces."

This must mean that two or more qualifiers for the
_same_ address space is allowed.

For dependent address space types, reject them like
before since we cannot know what the address space
will be.


Repository:
  rC Clang

https://reviews.llvm.org/D47630

Files:
  lib/Sema/SemaType.cpp
  test/Sema/address_spaces.c
  test/SemaOpenCL/address-spaces.cl


Index: test/SemaOpenCL/address-spaces.cl
===
--- test/SemaOpenCL/address-spaces.cl
+++ test/SemaOpenCL/address-spaces.cl
@@ -62,4 +62,6 @@
   __local __private int *var2;  // expected-error {{multiple address spaces 
specified for type}}
   __local private_int_t var3;   // expected-error {{multiple address spaces 
specified for type}}
   __local private_int_t *var4;  // expected-error {{multiple address spaces 
specified for type}}
+  __private private_int_t var5;
+  __private private_int_t *var6;
 }
Index: test/Sema/address_spaces.c
===
--- test/Sema/address_spaces.c
+++ test/Sema/address_spaces.c
@@ -14,6 +14,7 @@
 
   int _AS1 _AS2 *Y;   // expected-error {{multiple address spaces specified 
for type}}
   int *_AS1 _AS2 *Z;  // expected-error {{multiple address spaces specified 
for type}}
+  int *_AS1 _AS1 *M;
 
   _AS1 int local; // expected-error {{automatic variable qualified with an 
address space}}
   _AS1 int array[5];  // expected-error {{automatic variable qualified with an 
address space}}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -5703,14 +5703,6 @@
  SourceLocation AttrLoc) {
   if (!AddrSpace->isValueDependent()) { 
 
-// If this type is already address space qualified, reject it.
-// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified
-// by qualifiers for two or more different address spaces."
-if (T.getAddressSpace() != LangAS::Default) {
-  Diag(AttrLoc, diag::err_attribute_address_multiple_qualifiers);
-  return QualType();
-}
-
 llvm::APSInt addrSpace(32);
 if (!AddrSpace->isIntegerConstantExpr(addrSpace, Context)) {
   Diag(AttrLoc, diag::err_attribute_argument_type)
@@ -5741,6 +5733,16 @@
 LangAS ASIdx =
 getLangASFromTargetAS(static_cast(addrSpace.getZExtValue()));
 
+// If this type is already address space qualified with a different
+// address space, reject it.
+// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified
+// by qualifiers for two or more different address spaces."
+if (T.getAddressSpace() != LangAS::Default &&
+T.getAddressSpace() != ASIdx) {
+  Diag(AttrLoc, diag::err_attribute_address_multiple_qualifiers);
+  return QualType();
+}
+
 return Context.getAddrSpaceQualType(T, ASIdx);
   }
 
@@ -5762,15 +5764,6 @@
 /// space for the type.
 static void HandleAddressSpaceTypeAttribute(QualType &Type,
 const AttributeList &Attr, Sema 
&S){
-  // If this type is already address space qualified, reject it.
-  // ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified by
-  // qualifiers for two or more different address spaces."
-  if (Type.getAddressSpace() != LangAS::Default) {
-S.Diag(Attr.getLoc(), diag::err_attribute_address_multiple_qualifiers);
-Attr.setInvalid();
-return;
-  }
-
   // ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "A function type shall not be
   // qualified by an address-space qualifier."
   if (Type->isFunctionType()) {
@@ -5833,6 +5826,17 @@
   llvm_unreachable("Invalid address space");
 }
 
+// If this type is already address space qualified with a different
+// address space, reject it.
+// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified 
by
+// qualifiers for two or more different address spaces."
+if (Type.getAddressSpace() != LangAS::Default &&
+Type.getAddressSpace() != ASIdx) {
+  S.Diag(Attr.getLoc(), diag::err_attribute_address_multiple_qualifiers);
+  Attr.setInvalid();
+  return;
+}
+
 Type = S.Context.getAddrSpaceQualType(Type, ASIdx);
   }
 }


Index: test/SemaOpenCL/address-spaces.cl
===
--- test/SemaOpenCL/address-spaces.cl
+++ test/SemaOpenCL/address-spaces.cl
@@ -62,4 +62,6 @@
   __local __private int *var2;  // expected-error {{multiple address spaces specified for type}}
   __local private_int_t var3;   // expected-error {{multiple address spaces specified for type}}
   __local private_int_t *var4;  // expected-erro

[PATCH] D47630: [Sema] Allow creating types with multiple of the same addrspace.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: test/Sema/address_spaces.c:17
   int *_AS1 _AS2 *Z;  // expected-error {{multiple address spaces specified 
for type}}
+  int *_AS1 _AS1 *M;
 

bader wrote:
> I think it might be valuable to give a warning or remark to user. 
> Using the same address space qualifier multiple times is not something OpenCL 
> C developers are supposed to do.
> 
The test is obviously a bit contrived, but it could happen by mistake, or as a 
result of some typedef or macro combination. It also cannot go wrong, so 
there's no harm in it happening.

I see your point, though. A warning feels like a bit much, so I'm not sure what 
else to use. A note?


Repository:
  rC Clang

https://reviews.llvm.org/D47630



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47627: [ASTContext] Make getAddrSpaceQualType replace address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

I have this patch uploaded as well: https://reviews.llvm.org/D47630

I suspected that there might be cases for which we would try adding the same 
address space to a type, but I noticed that this method doesn't replace 
properly.
I don't actually have an example of this, though. It's possible that it will 
work anyway.


Repository:
  rC Clang

https://reviews.llvm.org/D47627



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47627: [ASTContext] Make getAddrSpaceQualType replace address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

There's actually a bit more to it than in these two patches. The complete diff 
to this function in our downstream Clang looks like this:

   QualType
   ASTContext::getAddrSpaceQualType(QualType T, unsigned AddressSpace) const {
  -  QualType CanT = getCanonicalType(T);
  -  if (CanT.getAddressSpace() == AddressSpace)
  +  if (T.getLocalQualifiers().getAddressSpace() == AddressSpace)
   return T;
   
 // If we are composing extended qualifiers together, merge together
 // into one ExtQuals node.
 QualifierCollector Quals;
 const Type *TypeNode = Quals.strip(T);
   
 // If this type already has an address space specified, it cannot get
 // another one.
  -  assert(!Quals.hasAddressSpace() &&
  - "Type cannot be in multiple addr spaces!");
  -  Quals.addAddressSpace(AddressSpace);
  +  if (Quals.hasAddressSpace())
  +Quals.removeAddressSpace();
  +  if (AddressSpace)
  +Quals.addAddressSpace(AddressSpace);
   
 return getExtQualType(TypeNode, Quals);
   }

In our downstream Clang, functions have address spaces. The desired address 
space is applied in getFunctionType, using getAddrSpaceQualType. Due to how 
FunctionTypes are built, it's possible to end up with noncanonical FunctionType 
that doesn't have an address space whose canonical type does have one. For 
example, when building the noncanonical type `void (*)(void * const)`, we will 
first build its canonical type `void (_AS *)(void *)`. Since 
getAddrSpaceQualType looks at the canonical type to determine whether the 
address space is already applied, it's skipped and we end up with this 
discrepancy.

Now that I test it again, I can't seem to induce the assertion. I suspect I 
might just have changed it because the documentation said that was how it was 
supposed to work. Perhaps I should be submitting the upper diff instead? 
Though, considering that this cannot be reproduced in trunk maybe I simply 
shouldn't submit it at all.


Repository:
  rC Clang

https://reviews.llvm.org/D47627



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46911: [Fixed Point Arithmetic] Addition of the remaining fixed point types and their saturated equivalents

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

Yes, it looks good to me.


Repository:
  rC Clang

https://reviews.llvm.org/D46911



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: include/clang/AST/Type.h:6552
+// as a scaled integer.
+std::string FixedPointValueToString(unsigned Radix, unsigned Scale,
+uint64_t Val);

This should probably take an APInt or APSInt.

Also, is there a reason to only have it produce unsigned numbers?



Comment at: include/clang/Basic/LangOptions.def:306
 LANGOPT(FixedPoint, 1, 0, "fixed point types")
+LANGOPT(SameFBits, 1, 0,
+"unsigned and signed fixed point type having the same number of 
fractional bits")

Is it safe to have this as a flag? What about making it a target property?



Comment at: include/clang/Basic/TargetInfo.h:89
+  // corresponding unsaturated types.
+  unsigned char ShortAccumFBits, ShortAccumIBits;
+  unsigned char AccumFBits, AccumIBits;

I suspect it's still possible to calculate the ibits based on the fbits, even 
for _Accum.

Are the unsigned values needed? The fbits for unsigned _Fract are the same as 
for signed _Fract if SameFBits is set, and +1 otherwise. The same should go for 
unsigned _Accum, but I don't think it's entirely clear how this affects the 
integral part.



Comment at: include/clang/Lex/LiteralSupport.h:116
+  /// occurred when calculating the integral part of the scaled integer.
+  bool GetFixedPointValue(uint64_t &Val, unsigned Scale);
+

This should return an APInt. That way you won't run the risk of losing any bits 
due to overflow or extraneous precision.



Comment at: lib/AST/ExprConstant.cpp:9427
+  const APSInt &Value = Result.getInt();
+  if (Value.isSigned() && Value.isMinSignedValue() && E->canOverflow()) {
+std::string Val =

This doesn't saturate properly. Is that coming in a later patch?



Comment at: lib/AST/ExprConstant.cpp:9429
+std::string Val =
+"-" + FixedPointValueToString(
+  /*Radix=*/10, Info.Ctx.getTypeInfo(E->getType()).Width,

This would probably be simpler if the FixedPointValueToString function could 
produce signed values.



Comment at: lib/AST/StmtPrinter.cpp:1540
+  llvm_unreachable("Unexpected type for fixed point literal!");
+case BuiltinType::ShortFract:
+  OS << "hr";

Format this more like the one above to make fewer lines.



Comment at: lib/AST/Type.cpp:3992
+
+std::string clang::FixedPointValueToString(unsigned Radix, unsigned Scale,
+   uint64_t Val) {

I think this would be better if it took a SmallString (or SmallVectorImpl) as 
reference and used that instead of returning a std::string.

Also should probably take an APInt/APSInt.



Comment at: lib/Lex/LiteralSupport.cpp:1045
 
+bool NumericLiteralParser::GetFixedPointValue(uint64_t &Val, unsigned Scale) {
+  assert(radix == 16 || radix == 10);

This should take an APInt (and use APInts for processing) to store the result 
in.

It should be possible to calculate exactly how many bits are needed to fit the 
literal before you start reading the digits. Overflow should not be a problem, 
but you might get precision loss in the fractional part. The calculation in our 
version is `ceil(log2(10^(noof-digits))) + Scale` but ours only handles normal 
decimal notation (123.456) so it might need to be extended to handle exponents 
and hex.



Comment at: lib/Sema/SemaExpr.cpp:1248
+  bool RHSFixed = RHSType->isFixedPointType();
+
+  if (LHSFixed && RHSFixed) {

I don't see how these semantics work properly. The specification requires that 
operations be done in the full precision of both types. You cannot convert the 
types before performing the operation like this, since the operation will not 
be done in full precision in that case.

The operator semantics of Embedded-C require the operand types of binary 
operators to be different. It's only when you've performed the operation that 
you are allowed to convert the result to the resulting type.



Comment at: lib/Sema/SemaExpr.cpp:1336
+  // Handle fixed point types
+  if (LHSType->isFixedPointType() || RHSType->isFixedPointType())
+return handleFixedPointConversion(*this, LHS, RHS, LHSType, RHSType,

I guess you haven't gotten there yet, but this should probably be before 
handleFloatConversion if you want to handle 'float + _fract' later.



Comment at: lib/Sema/SemaExpr.cpp:3415
+if (Literal.isFract) {
+  uint64_t MaxVal = 1ULL << scale;
+  if (Val > MaxVal) {

With APInts it should be possible to generalize this code to work with both 
accum and fract.


Repository:
  rC Clang

https://reviews.llvm.org/D46915



___
cfe-commits mailing 

[PATCH] D47630: [Sema] Allow creating types with multiple of the same addrspace.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: test/Sema/address_spaces.c:17
   int *_AS1 _AS2 *Z;  // expected-error {{multiple address spaces specified 
for type}}
+  int *_AS1 _AS1 *M;
 

Anastasia wrote:
> ebevhan wrote:
> > bader wrote:
> > > I think it might be valuable to give a warning or remark to user. 
> > > Using the same address space qualifier multiple times is not something 
> > > OpenCL C developers are supposed to do.
> > > 
> > The test is obviously a bit contrived, but it could happen by mistake, or 
> > as a result of some typedef or macro combination. It also cannot go wrong, 
> > so there's no harm in it happening.
> > 
> > I see your point, though. A warning feels like a bit much, so I'm not sure 
> > what else to use. A note?
> Just checked for const qualifier we get a warning:
>   warning: duplicate 'const' declaration specifier 
> [-Wduplicate-decl-specifier]
> 
> We could do the same... not sure if we could try to share the diagnostic as 
> well.
I have a patch ready that adds a warning in the same warning group and uses 
that. I'm not sure about reusing the other one since address spaces don't have 
to be declaration specifiers.

The warning is 'multiple identical address spaces specified for type', similar 
to the error. Is that acceptable?


Repository:
  rC Clang

https://reviews.llvm.org/D47630



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46911: [Fixed Point Arithmetic] Addition of the remaining fixed point types and their saturated equivalents

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

Actually, wait! One last thing I missed.




Comment at: include/clang/Sema/DeclSpec.h:670
const PrintingPolicy &Policy);
+  bool SetTypeSpecSat(SourceLocation Loc, const char *&PrevSpec,
+  unsigned &DiagID);

This should take a PrintingPolicy like the others.


Repository:
  rC Clang

https://reviews.llvm.org/D46911



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46911: [Fixed Point Arithmetic] Addition of the remaining fixed point types and their saturated equivalents

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: lib/Sema/SemaType.cpp:1612
+  // Only fixed point types can be saturated
+  if (DS.isTypeSpecSat() && !IsFixedPointType)
+S.Diag(DS.getTypeSpecSatLoc(), diag::err_invalid_saturation_spec)

Also, this does not seem to invalidate the declarator.


Repository:
  rC Clang

https://reviews.llvm.org/D46911



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46911: [Fixed Point Arithmetic] Addition of the remaining fixed point types and their saturated equivalents

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: include/clang/Sema/DeclSpec.h:670
const PrintingPolicy &Policy);
+  bool SetTypeSpecSat(SourceLocation Loc, const char *&PrevSpec,
+  unsigned &DiagID);

leonardchan wrote:
> ebevhan wrote:
> > This should take a PrintingPolicy like the others.
> I think the PrintingPolicy may not be necessary because it's only used for 
> getting the name of the current TypeSpecType. More specifically, for just 
> differentiating vetween "bool" and "_Bool" for `TST_bool`. It also seems that 
> other setters that don't touch TypeSpecType use PrintingPolicy, like 
> `SetTypeSpecComplex` or `SetTypeSpecSign`
You are correct, the ones that don't need it don't take it. I'm just being 
selfish since I need it downstream to disambiguate `__sat` and `_Sat`.

It's fine the way it is.



Comment at: lib/Sema/SemaType.cpp:1612
+  // Only fixed point types can be saturated
+  if (DS.isTypeSpecSat() && !IsFixedPointType)
+S.Diag(DS.getTypeSpecSatLoc(), diag::err_invalid_saturation_spec)

leonardchan wrote:
> ebevhan wrote:
> > Also, this does not seem to invalidate the declarator.
> How so? I have tests under `test/Frontend/fixed_point_errors.c` that check 
> for an error thrown if `_Sat` is used with non-fixed point types.
Hm, that is true. We don't have it downstream either. I'm not sure what the 
purpose of doing it is, but other invalid specifiers do 
`declarator.setInvalidType(true);`

I guess it isn't needed, just curious as to why it's done for some other ones.


Repository:
  rC Clang

https://reviews.llvm.org/D46911



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47630: [Sema] Allow creating types with multiple of the same addrspace.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan updated this revision to Diff 150683.
ebevhan edited the summary of this revision.
ebevhan added a comment.

Added a warning for identical address spaces.


https://reviews.llvm.org/D47630

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaType.cpp
  test/Sema/address_spaces.c
  test/SemaOpenCL/address-spaces.cl

Index: test/SemaOpenCL/address-spaces.cl
===
--- test/SemaOpenCL/address-spaces.cl
+++ test/SemaOpenCL/address-spaces.cl
@@ -62,4 +62,6 @@
   __local __private int *var2;  // expected-error {{multiple address spaces specified for type}}
   __local private_int_t var3;   // expected-error {{multiple address spaces specified for type}}
   __local private_int_t *var4;  // expected-error {{multiple address spaces specified for type}}
+  __private private_int_t var5; // expected-warning {{multiple identical address spaces specified for type}}
+  __private private_int_t *var6;// expected-warning {{multiple identical address spaces specified for type}}
 }
Index: test/Sema/address_spaces.c
===
--- test/Sema/address_spaces.c
+++ test/Sema/address_spaces.c
@@ -14,6 +14,7 @@
 
   int _AS1 _AS2 *Y;   // expected-error {{multiple address spaces specified for type}}
   int *_AS1 _AS2 *Z;  // expected-error {{multiple address spaces specified for type}}
+  int *_AS1 _AS1 *M;  // expected-warning {{multiple identical address spaces specified for type}}
 
   _AS1 int local; // expected-error {{automatic variable qualified with an address space}}
   _AS1 int array[5];  // expected-error {{automatic variable qualified with an address space}}
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -5703,14 +5703,6 @@
  SourceLocation AttrLoc) {
   if (!AddrSpace->isValueDependent()) { 
 
-// If this type is already address space qualified, reject it.
-// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified
-// by qualifiers for two or more different address spaces."
-if (T.getAddressSpace() != LangAS::Default) {
-  Diag(AttrLoc, diag::err_attribute_address_multiple_qualifiers);
-  return QualType();
-}
-
 llvm::APSInt addrSpace(32);
 if (!AddrSpace->isIntegerConstantExpr(addrSpace, Context)) {
   Diag(AttrLoc, diag::err_attribute_argument_type)
@@ -5741,6 +5733,20 @@
 LangAS ASIdx =
 getLangASFromTargetAS(static_cast(addrSpace.getZExtValue()));
 
+// If this type is already address space qualified with a different
+// address space, reject it.
+// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified
+// by qualifiers for two or more different address spaces."
+if (T.getAddressSpace() != LangAS::Default) {
+  if (T.getAddressSpace() != ASIdx) {
+Diag(AttrLoc, diag::err_attribute_address_multiple_qualifiers);
+return QualType();
+  } else
+// Emit a warning if they are identical; it's likely unintended.
+Diag(AttrLoc,
+ diag::warn_attribute_address_multiple_identical_qualifiers);
+}
+
 return Context.getAddrSpaceQualType(T, ASIdx);
   }
 
@@ -5762,15 +5768,6 @@
 /// space for the type.
 static void HandleAddressSpaceTypeAttribute(QualType &Type,
 const AttributeList &Attr, Sema &S){
-  // If this type is already address space qualified, reject it.
-  // ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified by
-  // qualifiers for two or more different address spaces."
-  if (Type.getAddressSpace() != LangAS::Default) {
-S.Diag(Attr.getLoc(), diag::err_attribute_address_multiple_qualifiers);
-Attr.setInvalid();
-return;
-  }
-
   // ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "A function type shall not be
   // qualified by an address-space qualifier."
   if (Type->isFunctionType()) {
@@ -5833,6 +5830,21 @@
   llvm_unreachable("Invalid address space");
 }
 
+// If this type is already address space qualified with a different
+// address space, reject it.
+// ISO/IEC TR 18037 S5.3 (amending C99 6.7.3): "No type shall be qualified by
+// qualifiers for two or more different address spaces."
+if (Type.getAddressSpace() != LangAS::Default) {
+  if (Type.getAddressSpace() != ASIdx) {
+S.Diag(Attr.getLoc(), diag::err_attribute_address_multiple_qualifiers);
+Attr.setInvalid();
+return;
+  } else
+// Emit a warning if they are identical; it's likely unintended.
+S.Diag(Attr.getLoc(),
+   diag::warn_attribute_address_multiple_identical_qualifiers);
+}
+
 Type = S.Context.getAddrSpaceQualType(Type, ASIdx);
   }
 }
Index: include/clang/Basic/DiagnosticSemaKinds.td
===

[PATCH] D47627: [ASTContext] Make getAddrSpaceQualType replace address spaces.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

> Well, the documentation mismatch is worth fixing even if the code isn't.  But 
> I think at best your use-case calls for weakening the assertion to be that 
> any existing address space isn't *different*, yeah.

Alright, I'll give that a shot.

> Separately, I'm not sure that's really the right representation for a Harvard 
> architecture (which is what I assume you're trying to extend Clang to 
> support); I think you should probably just teach the compiler that function 
> pointers are different.

Well, we've already implemented it and it's been running in our downstream for 
a while without issues at this point. We just figured it was less work to use 
the existing address space support for it than to hack special cases all over 
the place for functions and function pointers.


Repository:
  rC Clang

https://reviews.llvm.org/D47627



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: include/clang/Basic/TargetInfo.h:89
+  // corresponding unsaturated types.
+  unsigned char ShortAccumFBits, ShortAccumIBits;
+  unsigned char AccumFBits, AccumIBits;

leonardchan wrote:
> ebevhan wrote:
> > I suspect it's still possible to calculate the ibits based on the fbits, 
> > even for _Accum.
> > 
> > Are the unsigned values needed? The fbits for unsigned _Fract are the same 
> > as for signed _Fract if SameFBits is set, and +1 otherwise. The same should 
> > go for unsigned _Accum, but I don't think it's entirely clear how this 
> > affects the integral part.
> Similar to the previous comment, if we choose to make SameFBits dependent 
> on/controlled by the target, then I think it would be better for the target 
> to explicitly specify the integral bits since there could be some cases where 
> there may be padding (such as for unsigned _Accum types if this flag is set), 
> and in this case, I think it should be explicit that the `bit_width != IBits 
> + FBits`.
> 
> We also can't fill in that padding for the unsigned _Accum types as an extra 
> integral bit since the standard says that `"each signed accum type has at 
> least as many integral bits as its corresponding unsigned accum type".`
> 
> For the unsigned _Fract values, I think we can get rid of them if we choose 
> to keep the flag instead, but it would no longer apply to unsigned _Accum 
> types since it would allow for extra padding in these types and would 
> conflict with the logic of `bit_width == IBits + FBits`
> 
> For now, I actually think the simplest option is to keep these target 
> properties, but have the target override them individually, with checks to 
> make sure the values adhere to the standard.
Is it not the case that `bitwidth != IBits + FBits` for signed types only? 
Signed types require a sign bit (which is neither a fractional bit nor an 
integral bit, according to spec) but unsigned types do not, and if IBits and 
FBits for the signed and unsigned types are the same, the MSB is simply a 
padding bit, at least for _Accum (for _Fract everything above the fbits is 
padding). 

My reasoning for keeping the number of configurable values down was to limit 
the number of twiddly knobs to make the implementation simpler. Granting a lot 
of freedom is nice, but I suspect that it will be quite hard to get the 
implementation working properly for every valid configuration. I also don't 
really see much of a reason for `FBits != UFBits` in general. I know the spec 
gives a recommendation to implement it that way, but I think the benefit of 
normalizing the signed and unsigned representations outweighs the lost bit in 
the unsigned type.

It's hard to say what the differences are beyond that since I'm not sure how 
you plan on treating padding bits. In our implementation, padding bits (the MSB 
in all of the unsigned types for us) after any operation are zeroed.



Comment at: lib/Basic/TargetInfo.cpp:797
+  // corresponding unsigned accum type.
+  assert(ShortAccumIBits == UnsignedShortAccumIBits ||
+ ShortAccumIBits == UnsignedShortAccumIBits - 1);

Shouldn't these be `ShortAccumIBits >= UnsignedShortAccumIBits` etc.?



Comment at: lib/Lex/LiteralSupport.cpp:737
+  if (!hadError && saw_fixed_point_suffix) {
+assert(isFract || isAccum);
+assert(radix == 16 || radix == 10);

Is `saw_fixed_point_suffix` only used for this assertion? Doesn't `isFract || 
isAccum` imply `saw_fixed_point_suffix`?



Comment at: lib/Lex/LiteralSupport.cpp:1049
+
+bool NumericLiteralParser::GetFixedPointValue(llvm::APInt &StoreVal, unsigned 
Scale) {
+  assert(radix == 16 || radix == 10);

The function does a lot of overflow checks that I think are superfluous if the 
required bits are calculated properly. The only overflow check that should be 
needed is the one at the very bottom that truncates the APInt to the final size.



Comment at: lib/Lex/LiteralSupport.cpp:1065
+
+while (!IsExponentPart(*Ptr)) ++Ptr;
+++Ptr;

This whole block is very diligent but I wonder how common overflow in the 
exponent is.

I'm unsure if LLVM has a helper function with `atoll`-like functionality, but 
if it does it's a lot less code to just use that instead. There's also APInt's 
`fromString`, but that seems to assert if the integer doesn't fit in the 
bitwidth. The required bitwidth can be calculated here, though.



Comment at: lib/Lex/LiteralSupport.cpp:1085
+
+  if (radix == 10) {
+// Number of bits needed for decimal literal is

These two calculations look very similar to me. The only difference seems to be 
that the exponent is multiplied by 4 in the decimal case, and not in the 
hexadecimal case.



Comment at: lib/Lex/LiteralSupport.cpp:1141
+}
+if ((radix == 16 && (*Ptr ==

[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: lib/Lex/LiteralSupport.cpp:1187
+
+  uint64_t Base = (radix == 16) ? 2 : 10;
+  if (BaseShift > 0) {

leonardchan wrote:
> ebevhan wrote:
> > I don't think loops are needed here. BaseShift is essentially Exponent so 
> > it should be possible to implement as a `pow(radix, abs(BaseShift))` and 
> > then either a mul or a div based on whether or not it was positive or 
> > negative.
> There's a chance though that the value represented by `pow(radix, 
> abs(BaseShift))` may be too large to fit into an integer which is why used 
> the loop.
True, but I suspect that can be solved by making BaseShift an APInt with the 
same bitwidth as Val.



Comment at: lib/Sema/SemaExpr.cpp:1248
+  bool RHSFixed = RHSType->isFixedPointType();
+
+  if (LHSFixed && RHSFixed) {

leonardchan wrote:
> ebevhan wrote:
> > leonardchan wrote:
> > > ebevhan wrote:
> > > > I don't see how these semantics work properly. The specification 
> > > > requires that operations be done in the full precision of both types. 
> > > > You cannot convert the types before performing the operation like this, 
> > > > since the operation will not be done in full precision in that case.
> > > > 
> > > > The operator semantics of Embedded-C require the operand types of 
> > > > binary operators to be different. It's only when you've performed the 
> > > > operation that you are allowed to convert the result to the resulting 
> > > > type.
> > > Initially the idea was to convert both sides to fixed point types, then 
> > > perform standard binary operations between the fixed point types.
> > > 
> > > For the example, a `fract * int` would have the int converted to a fixed 
> > > point type by left shifting it by the scale of the fract, multiplying, 
> > > then right shifting by the scale again to get the resulting fract. The 
> > > only unhandled thing is overflow, but the precision of the fract remains 
> > > the same. The operands would also be casted up beforehand so there was 
> > > enough space to store the result, which was casted down back to the 
> > > original fract after performing the right shift by the scale.
> > > 
> > > Operations between fixed point types would follow a similar process of 
> > > casting both operands to the higher rank fixed point type, and depending 
> > > on the operation, more underlying shifting and casting would be done to 
> > > retain full precision of the higher ranked type.
> > > 
> > > Though I will admit that I did not realize until now that multiplying a 
> > > fixed point type by an integer does not require shifting the integer.
> > I see how you've reasoned; this is how C normally works. The `fract` is of 
> > higher rank than `int` and therefore is the 'common type' of the operation. 
> > However, even though it is higher rank there is no guarantee that you can 
> > perform the operation without overflowing. And overflow matters here; the 
> > spec says that it must be done in the full precision (integral + 
> > fractional) of both types.
> > 
> > > The only unhandled thing is overflow, but the precision of the fract 
> > > remains the same. The operands would also be casted up beforehand so 
> > > there was enough space to store the result, which was casted down back to 
> > > the original fract after performing the right shift by the scale.
> > 
> > The precision remains the same (and while it doesn't have to be the same to 
> > perform an operation, it makes the implementation more regular; things like 
> > addition and subtraction 'just work'), but you cannot perform a conversion 
> > to `fract` *before* the operation itself, since if you do, there's nothing 
> > to 'cast up'. Casting up is needed for things like `fract * fract` to 
> > prevent overflow, but for `fract * int` you need to cast to a type that can 
> > fit both all values of the int and all values of the fract, and *then* you 
> > can cast up before doing the multiplication.
> > 
> > > Operations between fixed point types would follow a similar process of 
> > > casting both operands to the higher rank fixed point type, and depending 
> > > on the operation, more underlying shifting and casting would be done to 
> > > retain full precision of the higher ranked type.
> > 
> > This might work, but I feel there could be edge cases. The E-C fixed-point 
> > ranks are very odd as they don't reflect reality; `short _Accum` cannot be 
> > considered strictly 'above' `long _Fract`, but the former has a higher rank 
> > than the latter. Depending on how the types are specified for a target, 
> > implicit casts between fixed-point types might inadvertantly discard bits, 
> > even though the spec says that operations must be done in full precision.
> I see, so just to confirm, something like a `fract * int` would not result in 
> any implicit casting between either operand, but any special arithmetic, like 
> intermediate storage types or satur

[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: include/clang/Basic/TargetInfo.h:89
+  // corresponding unsaturated types.
+  unsigned char ShortAccumFBits, ShortAccumIBits;
+  unsigned char AccumFBits, AccumIBits;

leonardchan wrote:
> ebevhan wrote:
> > leonardchan wrote:
> > > ebevhan wrote:
> > > > I suspect it's still possible to calculate the ibits based on the 
> > > > fbits, even for _Accum.
> > > > 
> > > > Are the unsigned values needed? The fbits for unsigned _Fract are the 
> > > > same as for signed _Fract if SameFBits is set, and +1 otherwise. The 
> > > > same should go for unsigned _Accum, but I don't think it's entirely 
> > > > clear how this affects the integral part.
> > > Similar to the previous comment, if we choose to make SameFBits dependent 
> > > on/controlled by the target, then I think it would be better for the 
> > > target to explicitly specify the integral bits since there could be some 
> > > cases where there may be padding (such as for unsigned _Accum types if 
> > > this flag is set), and in this case, I think it should be explicit that 
> > > the `bit_width != IBits + FBits`.
> > > 
> > > We also can't fill in that padding for the unsigned _Accum types as an 
> > > extra integral bit since the standard says that `"each signed accum type 
> > > has at least as many integral bits as its corresponding unsigned accum 
> > > type".`
> > > 
> > > For the unsigned _Fract values, I think we can get rid of them if we 
> > > choose to keep the flag instead, but it would no longer apply to unsigned 
> > > _Accum types since it would allow for extra padding in these types and 
> > > would conflict with the logic of `bit_width == IBits + FBits`
> > > 
> > > For now, I actually think the simplest option is to keep these target 
> > > properties, but have the target override them individually, with checks 
> > > to make sure the values adhere to the standard.
> > Is it not the case that `bitwidth != IBits + FBits` for signed types only? 
> > Signed types require a sign bit (which is neither a fractional bit nor an 
> > integral bit, according to spec) but unsigned types do not, and if IBits 
> > and FBits for the signed and unsigned types are the same, the MSB is simply 
> > a padding bit, at least for _Accum (for _Fract everything above the fbits 
> > is padding). 
> > 
> > My reasoning for keeping the number of configurable values down was to 
> > limit the number of twiddly knobs to make the implementation simpler. 
> > Granting a lot of freedom is nice, but I suspect that it will be quite hard 
> > to get the implementation working properly for every valid configuration. I 
> > also don't really see much of a reason for `FBits != UFBits` in general. I 
> > know the spec gives a recommendation to implement it that way, but I think 
> > the benefit of normalizing the signed and unsigned representations 
> > outweighs the lost bit in the unsigned type.
> > 
> > It's hard to say what the differences are beyond that since I'm not sure 
> > how you plan on treating padding bits. In our implementation, padding bits 
> > (the MSB in all of the unsigned types for us) after any operation are 
> > zeroed.
> I see. The implementation would be much simpler this way. The initial idea 
> was to treat the padding bits as "don't care" bits where we would mask only 
> the data bits when doing operations like comparisons that look at the whole 
> integer.
That does work, but for binary operations it means you might need multiple 
maskings per operation. If you mask after every operation instead, you only 
need one. In our implementation, the only padding bit we have is the MSB in the 
unsigned types. We simply insert a 'bit clear' after every operation that 
produces an unsigned type (with some exceptions).

The E-C spec says that padding bits are 'unspecified', but implementation-wise, 
they have to be defined to something in order to operate on them. So long as 
you ensure that it is never possible to observe anything but zero in those bits 
from a program perspective, most operations just work. Naturally, there are 
ways to subvert this design. If you have a `signed _Fract *` and cast it to an 
`unsigned _Fract *` and the values happen to be negative, the padding bit will 
be set and you'll get exciting behavior.

Although... I just realized that the topmost half of the `_Fract`s in your 
configuration is all padding. That might make things a bit more complicated for 
things like signed types... Ideally the sign bit would be extended into the 
padding for those.


Repository:
  rC Clang

https://reviews.llvm.org/D46915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46911: [Fixed Point Arithmetic] Addition of the remaining fixed point types and their saturated equivalents

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan accepted this revision.
ebevhan added a comment.
This revision is now accepted and ready to land.

LGTM, but I'm not a code owner so maybe someone else should chime in if they 
have any input.


Repository:
  rC Clang

https://reviews.llvm.org/D46911



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46911: [Fixed Point Arithmetic] Addition of the remaining fixed point types and their saturated equivalents

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: test/Frontend/fixed_point_bit_widths.c:44
+
+// CHECK-NEXT: @size_SsF  = dso_local global i{{[0-9]+}} 2
+// CHECK-NEXT: @size_SF   = dso_local global i{{[0-9]+}} 4

Wait; should these dso_local be `{{.*}}`?


Repository:
  rC Clang

https://reviews.llvm.org/D46911



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: lib/AST/ExprConstant.cpp:9437
+  }
+  return Success(-Value, E);
+}

This looks very suspect to me as well... This might not respect padding on 
types whose sign bit is not the MSB, such as _Fract. The same goes for the 
overflow check above.

I think it's quite important that we define the semantics of what padding bits 
should contain. Probably zeroes for unsigned types and a sexted sign bit for 
signed types.


Repository:
  rC Clang

https://reviews.llvm.org/D46915



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: include/clang/Basic/TargetInfo.h:89
+  // corresponding unsaturated types.
+  unsigned char ShortAccumFBits, ShortAccumIBits;
+  unsigned char AccumFBits, AccumIBits;

leonardchan wrote:
> ebevhan wrote:
> > leonardchan wrote:
> > > ebevhan wrote:
> > > > leonardchan wrote:
> > > > > ebevhan wrote:
> > > > > > I suspect it's still possible to calculate the ibits based on the 
> > > > > > fbits, even for _Accum.
> > > > > > 
> > > > > > Are the unsigned values needed? The fbits for unsigned _Fract are 
> > > > > > the same as for signed _Fract if SameFBits is set, and +1 
> > > > > > otherwise. The same should go for unsigned _Accum, but I don't 
> > > > > > think it's entirely clear how this affects the integral part.
> > > > > Similar to the previous comment, if we choose to make SameFBits 
> > > > > dependent on/controlled by the target, then I think it would be 
> > > > > better for the target to explicitly specify the integral bits since 
> > > > > there could be some cases where there may be padding (such as for 
> > > > > unsigned _Accum types if this flag is set), and in this case, I think 
> > > > > it should be explicit that the `bit_width != IBits + FBits`.
> > > > > 
> > > > > We also can't fill in that padding for the unsigned _Accum types as 
> > > > > an extra integral bit since the standard says that `"each signed 
> > > > > accum type has at least as many integral bits as its corresponding 
> > > > > unsigned accum type".`
> > > > > 
> > > > > For the unsigned _Fract values, I think we can get rid of them if we 
> > > > > choose to keep the flag instead, but it would no longer apply to 
> > > > > unsigned _Accum types since it would allow for extra padding in these 
> > > > > types and would conflict with the logic of `bit_width == IBits + 
> > > > > FBits`
> > > > > 
> > > > > For now, I actually think the simplest option is to keep these target 
> > > > > properties, but have the target override them individually, with 
> > > > > checks to make sure the values adhere to the standard.
> > > > Is it not the case that `bitwidth != IBits + FBits` for signed types 
> > > > only? Signed types require a sign bit (which is neither a fractional 
> > > > bit nor an integral bit, according to spec) but unsigned types do not, 
> > > > and if IBits and FBits for the signed and unsigned types are the same, 
> > > > the MSB is simply a padding bit, at least for _Accum (for _Fract 
> > > > everything above the fbits is padding). 
> > > > 
> > > > My reasoning for keeping the number of configurable values down was to 
> > > > limit the number of twiddly knobs to make the implementation simpler. 
> > > > Granting a lot of freedom is nice, but I suspect that it will be quite 
> > > > hard to get the implementation working properly for every valid 
> > > > configuration. I also don't really see much of a reason for `FBits != 
> > > > UFBits` in general. I know the spec gives a recommendation to implement 
> > > > it that way, but I think the benefit of normalizing the signed and 
> > > > unsigned representations outweighs the lost bit in the unsigned type.
> > > > 
> > > > It's hard to say what the differences are beyond that since I'm not 
> > > > sure how you plan on treating padding bits. In our implementation, 
> > > > padding bits (the MSB in all of the unsigned types for us) after any 
> > > > operation are zeroed.
> > > I see. The implementation would be much simpler this way. The initial 
> > > idea was to treat the padding bits as "don't care" bits where we would 
> > > mask only the data bits when doing operations like comparisons that look 
> > > at the whole integer.
> > That does work, but for binary operations it means you might need multiple 
> > maskings per operation. If you mask after every operation instead, you only 
> > need one. In our implementation, the only padding bit we have is the MSB in 
> > the unsigned types. We simply insert a 'bit clear' after every operation 
> > that produces an unsigned type (with some exceptions).
> > 
> > The E-C spec says that padding bits are 'unspecified', but 
> > implementation-wise, they have to be defined to something in order to 
> > operate on them. So long as you ensure that it is never possible to observe 
> > anything but zero in those bits from a program perspective, most operations 
> > just work. Naturally, there are ways to subvert this design. If you have a 
> > `signed _Fract *` and cast it to an `unsigned _Fract *` and the values 
> > happen to be negative, the padding bit will be set and you'll get exciting 
> > behavior.
> > 
> > Although... I just realized that the topmost half of the `_Fract`s in your 
> > configuration is all padding. That might make things a bit more complicated 
> > for things like signed types... Ideally the sign bit would be extended into 
> > the padding for those.
> Yeah, I initially had 2 general purpose ideas for handling the pad

[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:1248
+  bool RHSFixed = RHSType->isFixedPointType();
+
+  if (LHSFixed && RHSFixed) {

ebevhan wrote:
> leonardchan wrote:
> > ebevhan wrote:
> > > leonardchan wrote:
> > > > ebevhan wrote:
> > > > > I don't see how these semantics work properly. The specification 
> > > > > requires that operations be done in the full precision of both types. 
> > > > > You cannot convert the types before performing the operation like 
> > > > > this, since the operation will not be done in full precision in that 
> > > > > case.
> > > > > 
> > > > > The operator semantics of Embedded-C require the operand types of 
> > > > > binary operators to be different. It's only when you've performed the 
> > > > > operation that you are allowed to convert the result to the resulting 
> > > > > type.
> > > > Initially the idea was to convert both sides to fixed point types, then 
> > > > perform standard binary operations between the fixed point types.
> > > > 
> > > > For the example, a `fract * int` would have the int converted to a 
> > > > fixed point type by left shifting it by the scale of the fract, 
> > > > multiplying, then right shifting by the scale again to get the 
> > > > resulting fract. The only unhandled thing is overflow, but the 
> > > > precision of the fract remains the same. The operands would also be 
> > > > casted up beforehand so there was enough space to store the result, 
> > > > which was casted down back to the original fract after performing the 
> > > > right shift by the scale.
> > > > 
> > > > Operations between fixed point types would follow a similar process of 
> > > > casting both operands to the higher rank fixed point type, and 
> > > > depending on the operation, more underlying shifting and casting would 
> > > > be done to retain full precision of the higher ranked type.
> > > > 
> > > > Though I will admit that I did not realize until now that multiplying a 
> > > > fixed point type by an integer does not require shifting the integer.
> > > I see how you've reasoned; this is how C normally works. The `fract` is 
> > > of higher rank than `int` and therefore is the 'common type' of the 
> > > operation. However, even though it is higher rank there is no guarantee 
> > > that you can perform the operation without overflowing. And overflow 
> > > matters here; the spec says that it must be done in the full precision 
> > > (integral + fractional) of both types.
> > > 
> > > > The only unhandled thing is overflow, but the precision of the fract 
> > > > remains the same. The operands would also be casted up beforehand so 
> > > > there was enough space to store the result, which was casted down back 
> > > > to the original fract after performing the right shift by the scale.
> > > 
> > > The precision remains the same (and while it doesn't have to be the same 
> > > to perform an operation, it makes the implementation more regular; things 
> > > like addition and subtraction 'just work'), but you cannot perform a 
> > > conversion to `fract` *before* the operation itself, since if you do, 
> > > there's nothing to 'cast up'. Casting up is needed for things like `fract 
> > > * fract` to prevent overflow, but for `fract * int` you need to cast to a 
> > > type that can fit both all values of the int and all values of the fract, 
> > > and *then* you can cast up before doing the multiplication.
> > > 
> > > > Operations between fixed point types would follow a similar process of 
> > > > casting both operands to the higher rank fixed point type, and 
> > > > depending on the operation, more underlying shifting and casting would 
> > > > be done to retain full precision of the higher ranked type.
> > > 
> > > This might work, but I feel there could be edge cases. The E-C 
> > > fixed-point ranks are very odd as they don't reflect reality; `short 
> > > _Accum` cannot be considered strictly 'above' `long _Fract`, but the 
> > > former has a higher rank than the latter. Depending on how the types are 
> > > specified for a target, implicit casts between fixed-point types might 
> > > inadvertantly discard bits, even though the spec says that operations 
> > > must be done in full precision.
> > I see, so just to confirm, something like a `fract * int` would not result 
> > in any implicit casting between either operand, but any special arithmetic, 
> > like intermediate storage types or saturation handling, would be handled by 
> > the underlying IR?
> > 
> > So should really no conversions/implicit type casting should be performed 
> > here and instead all handling of arithmetic operations should happen 
> > somewhere during the codegen stage?
> > 
> > I see, so just to confirm, something like a fract * int would not result in 
> > any implicit casting between either operand, but any special arithmetic, 
> > like intermediate storage types or saturation handling, would be handled by 
> > the under

[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:1248
+  bool RHSFixed = RHSType->isFixedPointType();
+
+  if (LHSFixed && RHSFixed) {

leonardchan wrote:
> leonardchan wrote:
> > ebevhan wrote:
> > > ebevhan wrote:
> > > > leonardchan wrote:
> > > > > ebevhan wrote:
> > > > > > leonardchan wrote:
> > > > > > > ebevhan wrote:
> > > > > > > > I don't see how these semantics work properly. The 
> > > > > > > > specification requires that operations be done in the full 
> > > > > > > > precision of both types. You cannot convert the types before 
> > > > > > > > performing the operation like this, since the operation will 
> > > > > > > > not be done in full precision in that case.
> > > > > > > > 
> > > > > > > > The operator semantics of Embedded-C require the operand types 
> > > > > > > > of binary operators to be different. It's only when you've 
> > > > > > > > performed the operation that you are allowed to convert the 
> > > > > > > > result to the resulting type.
> > > > > > > Initially the idea was to convert both sides to fixed point 
> > > > > > > types, then perform standard binary operations between the fixed 
> > > > > > > point types.
> > > > > > > 
> > > > > > > For the example, a `fract * int` would have the int converted to 
> > > > > > > a fixed point type by left shifting it by the scale of the fract, 
> > > > > > > multiplying, then right shifting by the scale again to get the 
> > > > > > > resulting fract. The only unhandled thing is overflow, but the 
> > > > > > > precision of the fract remains the same. The operands would also 
> > > > > > > be casted up beforehand so there was enough space to store the 
> > > > > > > result, which was casted down back to the original fract after 
> > > > > > > performing the right shift by the scale.
> > > > > > > 
> > > > > > > Operations between fixed point types would follow a similar 
> > > > > > > process of casting both operands to the higher rank fixed point 
> > > > > > > type, and depending on the operation, more underlying shifting 
> > > > > > > and casting would be done to retain full precision of the higher 
> > > > > > > ranked type.
> > > > > > > 
> > > > > > > Though I will admit that I did not realize until now that 
> > > > > > > multiplying a fixed point type by an integer does not require 
> > > > > > > shifting the integer.
> > > > > > I see how you've reasoned; this is how C normally works. The 
> > > > > > `fract` is of higher rank than `int` and therefore is the 'common 
> > > > > > type' of the operation. However, even though it is higher rank 
> > > > > > there is no guarantee that you can perform the operation without 
> > > > > > overflowing. And overflow matters here; the spec says that it must 
> > > > > > be done in the full precision (integral + fractional) of both types.
> > > > > > 
> > > > > > > The only unhandled thing is overflow, but the precision of the 
> > > > > > > fract remains the same. The operands would also be casted up 
> > > > > > > beforehand so there was enough space to store the result, which 
> > > > > > > was casted down back to the original fract after performing the 
> > > > > > > right shift by the scale.
> > > > > > 
> > > > > > The precision remains the same (and while it doesn't have to be the 
> > > > > > same to perform an operation, it makes the implementation more 
> > > > > > regular; things like addition and subtraction 'just work'), but you 
> > > > > > cannot perform a conversion to `fract` *before* the operation 
> > > > > > itself, since if you do, there's nothing to 'cast up'. Casting up 
> > > > > > is needed for things like `fract * fract` to prevent overflow, but 
> > > > > > for `fract * int` you need to cast to a type that can fit both all 
> > > > > > values of the int and all values of the fract, and *then* you can 
> > > > > > cast up before doing the multiplication.
> > > > > > 
> > > > > > > Operations between fixed point types would follow a similar 
> > > > > > > process of casting both operands to the higher rank fixed point 
> > > > > > > type, and depending on the operation, more underlying shifting 
> > > > > > > and casting would be done to retain full precision of the higher 
> > > > > > > ranked type.
> > > > > > 
> > > > > > This might work, but I feel there could be edge cases. The E-C 
> > > > > > fixed-point ranks are very odd as they don't reflect reality; 
> > > > > > `short _Accum` cannot be considered strictly 'above' `long _Fract`, 
> > > > > > but the former has a higher rank than the latter. Depending on how 
> > > > > > the types are specified for a target, implicit casts between 
> > > > > > fixed-point types might inadvertantly discard bits, even though the 
> > > > > > spec says that operations must be done in full precision.
> > > > > I see, so just to confirm, something like a `fract * int` would not 
> > > > > result in any implicit casting between either operand, but any 
> > > > > special a

[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

Just a couple more comments and then I think it looks good.

We can discuss the conversion and comparison issues in later patches.




Comment at: include/clang/AST/ASTContext.h:1951
+  unsigned char getFixedPointScale(const QualType &Ty) const;
+  unsigned char getFixedPointIBits(const QualType &Ty) const;
+

These can probably take `QualType` directly.



Comment at: include/clang/Basic/TargetInfo.h:99
+  // sign if SameFBits is set.
+  unsigned char ShortAccumFBits;
+  unsigned char AccumFBits;

Could these and their accessors be called 'Scale' like in ASTContext? Only a 
consistency nit.



Comment at: lib/Sema/SemaExpr.cpp:1248
+  bool RHSFixed = RHSType->isFixedPointType();
+
+  if (LHSFixed && RHSFixed) {

leonardchan wrote:
> leonardchan wrote:
> > ebevhan wrote:
> > > ebevhan wrote:
> > > > leonardchan wrote:
> > > > > leonardchan wrote:
> > > > > > ebevhan wrote:
> > > > > > > ebevhan wrote:
> > > > > > > > leonardchan wrote:
> > > > > > > > > ebevhan wrote:
> > > > > > > > > > leonardchan wrote:
> > > > > > > > > > > ebevhan wrote:
> > > > > > > > > > > > I don't see how these semantics work properly. The 
> > > > > > > > > > > > specification requires that operations be done in the 
> > > > > > > > > > > > full precision of both types. You cannot convert the 
> > > > > > > > > > > > types before performing the operation like this, since 
> > > > > > > > > > > > the operation will not be done in full precision in 
> > > > > > > > > > > > that case.
> > > > > > > > > > > > 
> > > > > > > > > > > > The operator semantics of Embedded-C require the 
> > > > > > > > > > > > operand types of binary operators to be different. It's 
> > > > > > > > > > > > only when you've performed the operation that you are 
> > > > > > > > > > > > allowed to convert the result to the resulting type.
> > > > > > > > > > > Initially the idea was to convert both sides to fixed 
> > > > > > > > > > > point types, then perform standard binary operations 
> > > > > > > > > > > between the fixed point types.
> > > > > > > > > > > 
> > > > > > > > > > > For the example, a `fract * int` would have the int 
> > > > > > > > > > > converted to a fixed point type by left shifting it by 
> > > > > > > > > > > the scale of the fract, multiplying, then right shifting 
> > > > > > > > > > > by the scale again to get the resulting fract. The only 
> > > > > > > > > > > unhandled thing is overflow, but the precision of the 
> > > > > > > > > > > fract remains the same. The operands would also be casted 
> > > > > > > > > > > up beforehand so there was enough space to store the 
> > > > > > > > > > > result, which was casted down back to the original fract 
> > > > > > > > > > > after performing the right shift by the scale.
> > > > > > > > > > > 
> > > > > > > > > > > Operations between fixed point types would follow a 
> > > > > > > > > > > similar process of casting both operands to the higher 
> > > > > > > > > > > rank fixed point type, and depending on the operation, 
> > > > > > > > > > > more underlying shifting and casting would be done to 
> > > > > > > > > > > retain full precision of the higher ranked type.
> > > > > > > > > > > 
> > > > > > > > > > > Though I will admit that I did not realize until now that 
> > > > > > > > > > > multiplying a fixed point type by an integer does not 
> > > > > > > > > > > require shifting the integer.
> > > > > > > > > > I see how you've reasoned; this is how C normally works. 
> > > > > > > > > > The `fract` is of higher rank than `int` and therefore is 
> > > > > > > > > > the 'common type' of the operation. However, even though it 
> > > > > > > > > > is higher rank there is no guarantee that you can perform 
> > > > > > > > > > the operation without overflowing. And overflow matters 
> > > > > > > > > > here; the spec says that it must be done in the full 
> > > > > > > > > > precision (integral + fractional) of both types.
> > > > > > > > > > 
> > > > > > > > > > > The only unhandled thing is overflow, but the precision 
> > > > > > > > > > > of the fract remains the same. The operands would also be 
> > > > > > > > > > > casted up beforehand so there was enough space to store 
> > > > > > > > > > > the result, which was casted down back to the original 
> > > > > > > > > > > fract after performing the right shift by the scale.
> > > > > > > > > > 
> > > > > > > > > > The precision remains the same (and while it doesn't have 
> > > > > > > > > > to be the same to perform an operation, it makes the 
> > > > > > > > > > implementation more regular; things like addition and 
> > > > > > > > > > subtraction 'just work'), but you cannot perform a 
> > > > > > > > > > conversion to `fract` *before* the operation itself, since 
> > > > > > > > > > if you do, there's nothing to 'cast up'. Casting up is 
> > > > > > > > > > needed for things like `fract * fract` to prevent o

[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan accepted this revision.
ebevhan added inline comments.
This revision is now accepted and ready to land.



Comment at: lib/Sema/SemaExpr.cpp:1248
+  bool RHSFixed = RHSType->isFixedPointType();
+
+  if (LHSFixed && RHSFixed) {

leonardchan wrote:
> ebevhan wrote:
> > leonardchan wrote:
> > > leonardchan wrote:
> > > > ebevhan wrote:
> > > > > ebevhan wrote:
> > > > > > leonardchan wrote:
> > > > > > > leonardchan wrote:
> > > > > > > > ebevhan wrote:
> > > > > > > > > ebevhan wrote:
> > > > > > > > > > leonardchan wrote:
> > > > > > > > > > > ebevhan wrote:
> > > > > > > > > > > > leonardchan wrote:
> > > > > > > > > > > > > ebevhan wrote:
> > > > > > > > > > > > > > I don't see how these semantics work properly. The 
> > > > > > > > > > > > > > specification requires that operations be done in 
> > > > > > > > > > > > > > the full precision of both types. You cannot 
> > > > > > > > > > > > > > convert the types before performing the operation 
> > > > > > > > > > > > > > like this, since the operation will not be done in 
> > > > > > > > > > > > > > full precision in that case.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > The operator semantics of Embedded-C require the 
> > > > > > > > > > > > > > operand types of binary operators to be different. 
> > > > > > > > > > > > > > It's only when you've performed the operation that 
> > > > > > > > > > > > > > you are allowed to convert the result to the 
> > > > > > > > > > > > > > resulting type.
> > > > > > > > > > > > > Initially the idea was to convert both sides to fixed 
> > > > > > > > > > > > > point types, then perform standard binary operations 
> > > > > > > > > > > > > between the fixed point types.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > For the example, a `fract * int` would have the int 
> > > > > > > > > > > > > converted to a fixed point type by left shifting it 
> > > > > > > > > > > > > by the scale of the fract, multiplying, then right 
> > > > > > > > > > > > > shifting by the scale again to get the resulting 
> > > > > > > > > > > > > fract. The only unhandled thing is overflow, but the 
> > > > > > > > > > > > > precision of the fract remains the same. The operands 
> > > > > > > > > > > > > would also be casted up beforehand so there was 
> > > > > > > > > > > > > enough space to store the result, which was casted 
> > > > > > > > > > > > > down back to the original fract after performing the 
> > > > > > > > > > > > > right shift by the scale.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Operations between fixed point types would follow a 
> > > > > > > > > > > > > similar process of casting both operands to the 
> > > > > > > > > > > > > higher rank fixed point type, and depending on the 
> > > > > > > > > > > > > operation, more underlying shifting and casting would 
> > > > > > > > > > > > > be done to retain full precision of the higher ranked 
> > > > > > > > > > > > > type.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Though I will admit that I did not realize until now 
> > > > > > > > > > > > > that multiplying a fixed point type by an integer 
> > > > > > > > > > > > > does not require shifting the integer.
> > > > > > > > > > > > I see how you've reasoned; this is how C normally 
> > > > > > > > > > > > works. The `fract` is of higher rank than `int` and 
> > > > > > > > > > > > therefore is the 'common type' of the operation. 
> > > > > > > > > > > > However, even though it is higher rank there is no 
> > > > > > > > > > > > guarantee that you can perform the operation without 
> > > > > > > > > > > > overflowing. And overflow matters here; the spec says 
> > > > > > > > > > > > that it must be done in the full precision (integral + 
> > > > > > > > > > > > fractional) of both types.
> > > > > > > > > > > > 
> > > > > > > > > > > > > The only unhandled thing is overflow, but the 
> > > > > > > > > > > > > precision of the fract remains the same. The operands 
> > > > > > > > > > > > > would also be casted up beforehand so there was 
> > > > > > > > > > > > > enough space to store the result, which was casted 
> > > > > > > > > > > > > down back to the original fract after performing the 
> > > > > > > > > > > > > right shift by the scale.
> > > > > > > > > > > > 
> > > > > > > > > > > > The precision remains the same (and while it doesn't 
> > > > > > > > > > > > have to be the same to perform an operation, it makes 
> > > > > > > > > > > > the implementation more regular; things like addition 
> > > > > > > > > > > > and subtraction 'just work'), but you cannot perform a 
> > > > > > > > > > > > conversion to `fract` *before* the operation itself, 
> > > > > > > > > > > > since if you do, there's nothing to 'cast up'. Casting 
> > > > > > > > > > > > up is needed for things like `fract * fract` to prevent 
> > > > > > > > > > > > overflow, but for `fract * int` you need to cast to a 
> > > > > > > > > > > > type that can fit both all values o

[PATCH] D47630: [Sema] Allow creating types with multiple of the same addrspace.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

Thanks! I do not have commit access, so it would be great if someone could 
commit this.


https://reviews.llvm.org/D47630



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

[Bevin Hansson via Phabricator via cfe-commits]

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
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

[Bevin Hansson via Phabricator via cfe-commits]

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 point types.");

It's not in TargetCodeGenInfo any more.


Repository:
  rC Clang

https://reviews.llvm.org/D50616



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53308: [Fixed Point Arithmetic] Fixed Point to Boolean Cast

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1032
+  // them.
+  return Builder.CreateIsNotNull(Src);
+}

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

Maybe I'm misinterpreting what it means.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2023
+// We do not check the padding bit on unsigned types since they are zero'd
+// out on operations that can cause overflow into the bit.
+assert(E->getType()->isFixedPointType() &&

This comment is a duplicate of the one in EmitScalarConversion.



Comment at: clang/lib/Sema/Sema.cpp:537
   case Type::STK_FixedPoint:
-llvm_unreachable("Unknown cast from FixedPoint to boolean");
+return CK_FixedPointToBoolean;
   }

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


Repository:
  rC Clang

https://reviews.llvm.org/D53308



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53308: [Fixed Point Arithmetic] Fixed Point to Boolean Cast

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2026
+return EmitScalarConversion(Visit(E), E->getType(), DestTy,
+CE->getExprLoc());
 

rjmccall wrote:
> Why are you pushing these casts through `EmitScalarConversion` when the cast 
> kind already tells you which operation you're doing?
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.


Repository:
  rC Clang

https://reviews.llvm.org/D53308



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

I think I've already expressed that I'm not at all a fan of encoding the 
full-precision logic in the operations themselves and not explicitly in the 
AST. We already have (well, not yet, but we will) routines for emitting 
conversions to/from/between fixed-point types of arbitrary semantics - why not 
use that instead of reimplementing the same logic for every binary operation?

As it is now, EmitFixedPointAdd replicates the logic for both converting from 
integer to fixed-point and fixed-point to fixed-point. You mention a special 
case with saturating addition, but this special case only exists because the 
routine needs to do fixed-point->saturating fixed-point on its own. If all 
EmitFixedPointAdd did was perform an add between two fixed-point values with 
the same semantics, then the logic would be much simpler and the act of 
conversion would be delegated to the routines that actually handle it.

I want to float the idea again of adding an AST type to encapsulate an 
arbitrary fixed-point semantic and using that as the 'common type' for binary 
operations involving fixed-point types. This would enable 
UsualArithmeticConversions to handle fixed-point types similarly to how it does 
today (find the 'common' full precision type, implicitly cast the LHS and RHS 
if necessary). There is one new thing that it would have to do; the result 
after the operation should not be the full precision type, but rather be casted 
to the operand type of highest rank. I don't think the existing code in 
SemaExpr can handle the case of adding an implicit cast after the operation. I 
don't think it should be hard to add, though.




Comment at: clang/lib/Sema/SemaExpr.cpp:1393
+/// rules in N1169 4.1.4.
+QualType Sema::FixedPointConversions(ExprResult &FixedExpr,
+ ExprResult &OtherExpr, bool IsCompAssign) 
{

I'm not sure I understand why this is in a separate function. What part of this 
doesn't work in UsualArithmeticConversions, in a 'handleFixedPointConversion' 
similar to the other cases?



Comment at: clang/lib/Sema/SemaExpr.cpp:1416
+  // converted to its corresponding signed fixed-point type and the resulting
+  // type is the type of the converted operand.
+  if (OtherTy->isSignedFixedPointType() &&

I feel like this logic can be folded into the rank handling. Does it work 
properly if you give signed types higher rank than unsigned ones?



Comment at: clang/lib/Sema/SemaExpr.cpp:1435
+  // account rounding and overflow) to the precision of the resulting type.
+  if (OtherTy->isIntegerType())
+return FixedTy;

This can be avoided by making all integer types lower rank than the fixed point 
types. The rank between them doesn't matter, either; they can all be the same.


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

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In https://reviews.llvm.org/D53738#1278078, @rjmccall wrote:

> I don't think we should add *types* just for this, but if you need to make a 
> different kind of `BinaryOperator` that represents that the semantics aren't 
> quite the same (the same way that the compound assignments have their own 
> subclass), that seems natural to me.


I don't know if adding a new operator kind was what I had in mind. The operator 
hasn't changed, it's still a normal binary operator. Compound assignments are 
their own operator with its own syntax; it doesn't really strike me as the same 
thing.

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".


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

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

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/D53

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In https://reviews.llvm.org/D53738#1281332, @rjmccall wrote:

> Well, maybe the cleanest solution would be to actually fold 
> `CompoundAssignOperator` back into `BinaryOperator` and just allow 
> `BinaryOperator` to optionally store information about the intermediate type 
> of the computation and how the operands need to be promoted.  That 
> information could be elided in the common cases where it's trivial, of course.


That sounds like a fairly hefty refactor. Also, doing the fold would put the 
extra QualType info from CAO into BO, sure, but this cannot work for the 
full-precision case since we can't represent those with QualTypes. The 
information for the full precision 'type' would have to be stored separately 
anyway.

Or did you mean to make a subclass of that new BinaryOperator for the full 
precision case, and store the full precision info there?

It might just be easier to store the full-precision info in BO directly. BO 
might be too common to warrant the size increase, though. FixedPointSemantics 
can probably be optimized to only take 32 bits.

> The infinite-precision rule here is still internal to an individual operator, 
> right?  The standard's not trying to say that we should evaluate `x + y < z` 
> by doing a comparison as if all the operands were individually 
> infinite-precision?

Correct, the result of the computation is 'implicitly converted' back to the 
result type after the operation is performed. The type of the expression will 
always be the result type, never the full precision type.

As a side note, comparisons are still a bit up in the air. I don't think we 
came to a conclusion on whether they should be done in full precision or 
bitwise. The spec isn't clear.


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

[PATCH] D55868: [Fixed Point Arithmetic] Fixed Point Addition Constant Expression Evaluation

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:7568
 
+static bool EvaluateFixedPoint(const Expr *E, APValue &Result, EvalInfo &Info) 
{
+  if (E->getType()->isFixedPointType()) {

This could provide an APFixedPoint rather than APValue.



Comment at: clang/lib/AST/ExprConstant.cpp:7582
+
+static bool EvaluateFixedPointOrInteger(const Expr *E, APValue &Result,
+EvalInfo &Info) {

Technically, this will always produce an APFixedPoint so it doesn't really need 
to return APValue either.



Comment at: clang/lib/AST/ExprConstant.cpp:7584
+EvalInfo &Info) {
+  auto FXSema = Info.Ctx.getFixedPointSemantics(E->getType());
+  if (E->getType()->isIntegerType()) {

The semantic is not used in the fixed-point path.



Comment at: clang/lib/AST/ExprConstant.cpp:9927
   }
   return Success(-Value, E);
 }

I think I've mentioned this before, but just as a reminder; this doesn't 
perform correctly for saturation.



Comment at: clang/lib/AST/ExprConstant.cpp:9979
+APFixedPoint Result =
+LHSFX.getFixedPoint().add(RHSFX.getFixedPoint()).convert(ResultFXSema);
+return Success(Result, E);

Perhaps this should call HandleOverflow (or do something else) to signal that 
overflow occurred. HandleOverflow only seems to emit a note, though, but I 
think it should probably be a warning.

Maybe for casts as well? Might get double warnings then, though.



Comment at: clang/lib/Basic/FixedPoint.cpp:25
   bool Upscaling = DstScale > getScale();
+  bool Overflowed = false;
 

I think it's a bit cleaner if you avoid this variable and simply assign 
`Overflow` directly here, with the 'else' cases below replaced with `else if 
(Overflow)`.

That style isn't cleaner in `add` if you use the APInt add_ov functions though, 
so maybe it's better to keep it this way for consistency.



Comment at: clang/lib/Basic/FixedPoint.cpp:170
+Result = ThisVal + OtherVal;
+Overflowed = ThisVal.isSigned() ? Result.slt(ThisVal) : 
Result.ult(ThisVal);
+  }

You could use the add_ov functions here.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55868/new/

https://reviews.llvm.org/D55868



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56900: [Fixed Point Arithmetic] Fixed Point and Integer Conversions

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:9825
+if (Result.isSigned() && !DstSigned) {
+  Overflow = Result < 0 || Result.ugt(DstMax);
+} else if (Result.isUnsigned() && DstSigned) {

The `Result < 0` is more clearly expressed as `Result.isNegative()` I think.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1499
+  IsNegFractional, NegResult,
+  Builder.CreateAShr(Result, SrcScale - DstScale, "downscale"));
+} else {

I think this block can be simplified significantly by simply rounding up before 
conversion. So, something like:
```
%lt = icmp slt %val, 0
%rounded = add %val, lowBits(Scale)
%sel = select %lt, %rounded, %val
... rescale/resize ...
```




Comment at: clang/lib/Sema/SemaChecking.cpp:11096
+}
+  }
 }

Seems like a lot of logic in these blocks is duplicated from the code in 
ExprConstant.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56900/new/

https://reviews.llvm.org/D56900



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56900: [Fixed Point Arithmetic] Fixed Point and Integer Conversions

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: clang/lib/AST/ExprConstant.cpp:9825
+if (Result.isSigned() && !DstSigned) {
+  Overflow = Result < 0 || Result.ugt(DstMax);
+} else if (Result.isUnsigned() && DstSigned) {

leonardchan wrote:
> ebevhan wrote:
> > The `Result < 0` is more clearly expressed as `Result.isNegative()` I think.
> Ah, so I ran into something similar with the patch preceding this in 
> `APFixedPoint::convert()`. `isNegative()` is a method of `APInt` which 
> doesn't care about signage. It just checks if the last bit is set. `Result < 
> 0` calls `APSInt::operator<()` which cares about the sign and checks if this 
> signed int is less than zero. 
> 
>  have no big problem with this, but if `Result.isNegative()` is more 
> readable, I could also add `Result.isSigned()` which together effectively 
> does the same thing as `Result < 0`.
This makes sense, but you're already checking if the value is signed in the 
line above, so it shouldn't be an issue.



Comment at: clang/test/Frontend/fixed_point_conversions.c:426
+  _Sat short _Accum sat_sa;
+  _Sat unsigned short _Accum sat_usa;
+

There are no tests here for what you get if you convert an integer to a 
fixed-point type with a larger integral part than the integer has.



Comment at: clang/test/Frontend/fixed_point_conversions.c:437
+  // DEFAULT-NEXT: [[RES:%[a-z0-9]+]] = trunc i39 [[SATMIN]] to i16
+  // DEFAULT-NEXT: store i16 [[RES]], i16* %sat_sa, align 2
+

Conversions like this are a bit odd. There shouldn't be a need to 
upsize/upscale the container before the saturation, should there?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56900/new/

https://reviews.llvm.org/D56900



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

I'm a bit late to the party here, it was an older patch so I wasn't watching 
this one.

I get a bit miffed when address space related features get locked behind 
langoptions that aren't strictly address spaces. I know that there are 
currently a couple code snippets which are behind LangOptions.OpenCL, that are 
needed for address spaces to work reasonably even when you aren't using OpenCL.

I guess I do understand that the only address spaces that are interesting to 
parse here are the named qualifier ones, but it would be convenient if the 
parsing would accept other ones as well (such as the `__attribute__` based 
ones) and not necessarily assume that the user is using OpenCL++.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55850/new/

https://reviews.llvm.org/D55850



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

Okay, sounds good! I'm not a C++ expert, but I'd be happy to look at the 
patches when they're up. Haven't done much serious testing on my end so far, 
but from what I've seen the patches so far look good!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55850/new/

https://reviews.llvm.org/D55850



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

I think the code that comes to mind is mostly like in 
`GetTypeSourceInfoForDeclarator`:

  LangAS AS =
  (ASIdx == LangAS::Default ? LangAS::opencl_generic : ASIdx);

It's behind an OpenCLCPlusPlus guard so it won't add generic on anything if 
it's not OpenCL++, but there shouldn't be a reason why the rest of the code in 
that block won't work for regular C++.

In fact, in most regular C++ this would be an issue, since Default typically 
_is_ the 'generic' address space there.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55850/new/

https://reviews.llvm.org/D55850



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: lib/Sema/SemaOverload.cpp:5151
 
+  if (FromTypeCanon.getQualifiers().hasAddressSpace()) {
+Qualifiers QualsImplicitParamType = ImplicitParamType.getQualifiers();

I tested this patch with some kludges to let it parse our address space 
qualifiers, and hit a problem here; just because the FromType isn't qualified 
doesn't mean that the object conversion is okay. A conversion of an object 
`Type` to `__X Type` might not be legal if there is no conversion from 'no 
address space' to '__X' address space.

The example was a class with an AS-qualified overload, and when resolving for 
calling the overloaded method on an object `Type *`, it hit an ambiguous 
resolution since it considered the AS-qualified method to be a legal candidate.

It feels like this might be rather specific to how a language/target wants to 
use address spaces, though.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55850/new/

https://reviews.llvm.org/D55850



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D56900: [Fixed Point Arithmetic] Fixed Point and Integer Conversions

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: clang/test/Frontend/fixed_point_conversions.c:437
+  // DEFAULT-NEXT: [[RES:%[a-z0-9]+]] = trunc i39 [[SATMIN]] to i16
+  // DEFAULT-NEXT: store i16 [[RES]], i16* %sat_sa, align 2
+

leonardchan wrote:
> ebevhan wrote:
> > Conversions like this are a bit odd. There shouldn't be a need to 
> > upsize/upscale the container before the saturation, should there?
> I see. You're saying that we can just check directly if the value exceeds 255 
> (or goes under -256) since the range of target values can always fit in the 
> range of source values. Therefore we do not need to cast up since the only 
> reason we would need to is if converting to a type with a greater source 
> range.
> 
> In this case, we could technically have a special case for integers where I 
> think we can perform the saturation checks without the initial sign 
> extension. I think it might be simpler though if in 
> `EmitFixedPointConversion`, we treat an integer as a 'zero-scale fixed point 
> number'. I think the reason the upsizing occurs in the first place is so that 
> we satisfy the first FX conversion rule of calculating the result with full 
> precision of both operands.
> I see. You're saying that we can just check directly if the value exceeds 255 
> (or goes under -256) since the range of target values can always fit in the 
> range of source values. Therefore we do not need to cast up since the only 
> reason we would need to is if converting to a type with a greater source 
> range.

That's right. Though, for integer to fixed-point conversion, I don't think you 
ever need to upcast first; either the integer is larger than or equal to the 
integral part of the fixed-point type, in which case we can check the magnitude 
in the type as is, or it's smaller, and we don't have to do any saturation.

> I think it might be simpler though if in `EmitFixedPointConversion`, we treat 
> an integer as a 'zero-scale fixed point number'.

Isn't this already the case? The semantics for an integer type are fetched as a 
zero scale fixed-point type and used that way (except when the target is an 
integer due to the rounding requirement).


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56900/new/

https://reviews.llvm.org/D56900



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57219: [Fixed Point Arithmetic] Fixed Point Comparisons

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:141
+ (RHSType->isFixedPointType() &&
+  LHSType->isFixedPointOrIntegerType());
+}

Can't it just be `LHSType->isFixedPointType() || RHSType->isFixedPointType()`?

I don't think there are cases where one is a fixed-point type and the other is 
not an integer or another fixed-point type, so if one is fixed-point then you 
already know it's a fixed-point operation.



Comment at: clang/test/Frontend/fixed_point_comparisons.c:56
+
+void TestComparisons() {
+  short _Accum sa;

Missing saturating and saturating/non-saturating comparisons. I'd like to see 
the differences between unsigned padding and not there, if there are any.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57219/new/

https://reviews.llvm.org/D57219



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57226: [Fixed Point] [AST] Add an AST serialization code for fixed-point literals.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan created this revision.
ebevhan added reviewers: leonardchan, rjmccall.
Herald added a subscriber: cfe-commits.
ebevhan marked an inline comment as done.
ebevhan added inline comments.



Comment at: include/clang/Serialization/ASTBitCodes.h:1637
+  /// A FixedPointLiteral record.
+  EXPR_FIXEDPOINT_LITERAL,
+

I'm unsure if this is the right location for a new code. This will bump down 
all the other codes below this and cause any older file to not be read 
correctly.

Should the file format version number be bumped up?


This patch adds the EXPR_FIXEDPOINT_LITERAL AST
code to serialize FixedPointLiterals. They were previously
being serialized with the code for integer literals, which
doesn't work properly.


Repository:
  rC Clang

https://reviews.llvm.org/D57226

Files:
  include/clang/AST/Expr.h
  include/clang/Serialization/ASTBitCodes.h
  lib/AST/Expr.cpp
  lib/Serialization/ASTReaderStmt.cpp
  lib/Serialization/ASTWriter.cpp
  lib/Serialization/ASTWriterStmt.cpp
  test/PCH/fixed-point-literal.c
  test/PCH/fixed-point-literal.h

Index: test/PCH/fixed-point-literal.h
===
--- /dev/null
+++ test/PCH/fixed-point-literal.h
@@ -0,0 +1,5 @@
+// Header for PCH test fixed-point-literal.c
+
+const short _Fract sf = -0.25r;
+const _Fract f = 0.75r;
+const long _Accum la = 25.25lk;
Index: test/PCH/fixed-point-literal.c
===
--- /dev/null
+++ test/PCH/fixed-point-literal.c
@@ -0,0 +1,14 @@
+// Test this without pch.
+// RUN: %clang_cc1 -ffixed-point -include %S/fixed-point-literal.h -fsyntax-only -ast-print -o - %s | FileCheck %s
+
+// Test with pch.
+// RUN: %clang_cc1 -ffixed-point -emit-pch -o %t %S/fixed-point-literal.h
+// RUN: %clang_cc1 -ffixed-point -include-pch %t -fsyntax-only -ast-print -o - %s | FileCheck %s
+
+// CHECK: const short _Fract sf = -0.25r;
+// CHECK: const _Fract f = 0.75r;
+// CHECK: const long _Accum la = 25.25lk;
+
+short _Fract sf2 = sf;
+_Fract f2 = f;
+long _Accum la2 = la;
Index: lib/Serialization/ASTWriterStmt.cpp
===
--- lib/Serialization/ASTWriterStmt.cpp
+++ lib/Serialization/ASTWriterStmt.cpp
@@ -496,8 +496,9 @@
 void ASTStmtWriter::VisitFixedPointLiteral(FixedPointLiteral *E) {
   VisitExpr(E);
   Record.AddSourceLocation(E->getLocation());
+  Record.push_back(E->getScale());
   Record.AddAPInt(E->getValue());
-  Code = serialization::EXPR_INTEGER_LITERAL;
+  Code = serialization::EXPR_FIXEDPOINT_LITERAL;
 }
 
 void ASTStmtWriter::VisitFloatingLiteral(FloatingLiteral *E) {
Index: lib/Serialization/ASTWriter.cpp
===
--- lib/Serialization/ASTWriter.cpp
+++ lib/Serialization/ASTWriter.cpp
@@ -958,6 +958,7 @@
   RECORD(EXPR_PREDEFINED);
   RECORD(EXPR_DECL_REF);
   RECORD(EXPR_INTEGER_LITERAL);
+  RECORD(EXPR_FIXEDPOINT_LITERAL);
   RECORD(EXPR_FLOATING_LITERAL);
   RECORD(EXPR_IMAGINARY_LITERAL);
   RECORD(EXPR_STRING_LITERAL);
Index: lib/Serialization/ASTReaderStmt.cpp
===
--- lib/Serialization/ASTReaderStmt.cpp
+++ lib/Serialization/ASTReaderStmt.cpp
@@ -576,6 +576,7 @@
 void ASTStmtReader::VisitFixedPointLiteral(FixedPointLiteral *E) {
   VisitExpr(E);
   E->setLocation(ReadSourceLocation());
+  E->setScale(Record.readInt());
   E->setValue(Record.getContext(), Record.readAPInt());
 }
 
@@ -2471,6 +2472,10 @@
   S = IntegerLiteral::Create(Context, Empty);
   break;
 
+case EXPR_FIXEDPOINT_LITERAL:
+  S = FixedPointLiteral::Create(Context, Empty);
+  break;
+
 case EXPR_FLOATING_LITERAL:
   S = FloatingLiteral::Create(Context, Empty);
   break;
Index: lib/AST/Expr.cpp
===
--- lib/AST/Expr.cpp
+++ lib/AST/Expr.cpp
@@ -818,6 +818,11 @@
   return new (C) FixedPointLiteral(C, V, type, l, Scale);
 }
 
+FixedPointLiteral *FixedPointLiteral::Create(const ASTContext &C,
+ EmptyShell Empty) {
+  return new (C) FixedPointLiteral(Empty);
+}
+
 std::string FixedPointLiteral::getValueAsString(unsigned Radix) const {
   // Currently the longest decimal number that can be printed is the max for an
   // unsigned long _Accum: 4294967295.976716935634613037109375
Index: include/clang/Serialization/ASTBitCodes.h
===
--- include/clang/Serialization/ASTBitCodes.h
+++ include/clang/Serialization/ASTBitCodes.h
@@ -1633,6 +1633,9 @@
   /// An IntegerLiteral record.
   EXPR_INTEGER_LITERAL,
 
+  /// A FixedPointLiteral record.
+  EXPR_FIXEDPOINT_LITERAL,
+
   /// A FloatingLiteral record.
   EXPR_FLOATING_LITERAL,
 
Index: include/clang/AST/Expr.h

[PATCH] D57226: [Fixed Point] [AST] Add an AST serialization code for fixed-point literals.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan marked an inline comment as done.
ebevhan added inline comments.



Comment at: include/clang/Serialization/ASTBitCodes.h:1637
+  /// A FixedPointLiteral record.
+  EXPR_FIXEDPOINT_LITERAL,
+

I'm unsure if this is the right location for a new code. This will bump down 
all the other codes below this and cause any older file to not be read 
correctly.

Should the file format version number be bumped up?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57226/new/

https://reviews.llvm.org/D57226



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57219: [Fixed Point Arithmetic] Fixed Point Comparisons

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: clang/test/Frontend/fixed_point_comparisons.c:56
+
+void TestComparisons() {
+  short _Accum sa;

leonardchan wrote:
> ebevhan wrote:
> > Missing saturating and saturating/non-saturating comparisons. I'd like to 
> > see the differences between unsigned padding and not there, if there are 
> > any.
> I don't think there should be since we compare by converting to a common type 
> that should fit both operands, but it does help to have tests that also 
> confirm this. Added some saturation cases under `TestSaturationComparisons`.
> 
> As for padding, `TestSaturationComparisons` have cases comparing signed with 
> unsigned types, and there are other cases in `TestComparisons` and 
> `TestIntComparisons` that deal with unsigned comparisons. The way the lit 
> tests are organized, lines marked with `CHECK` mean that those lines are 
> produced for both the padding and no padding cases whereas `SIGNED` or 
> `UNSIGNED` are produced exclusively for no padding and padding cases, 
> respectively.
There is a difference between saturating signed types and saturating unsigned 
types, though; the common type of two of the same saturating unsigned type is 
one bit less due to padding.

Unless there is something in the type commoning that I've missed?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57219/new/

https://reviews.llvm.org/D57219



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57464: Generalize method overloading on addr spaces to C++

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: lib/Parse/ParseDeclCXX.cpp:2309
+  attr.getScopeName(), attr.getScopeLoc(), &AU,
+  attr.getNumArgs(), ParsedAttr::AS_GNU);
+attr.setInvalid();

Are we interested in preserving the spelling?



Comment at: lib/Parse/ParseDeclCXX.cpp:2313
+}
+  }
 

Is there a reason that the attributes are parsed here and not in 
`ParseFunctionDeclarator` like the rest of the ref-qualifiers (and the OpenCL++ 
ASes)?

That is possibly why the parser is getting confused with the trailing return.



Comment at: lib/Sema/SemaType.cpp:4883
+  return AS;
+}};
+  LangAS AS = LangAS::Default;

This lambda is a bit unwieldy, it would probably be better as a separate 
function.



Comment at: lib/Sema/SemaType.cpp:5865
+/// value and argument expression will be filled in. Returns value indicating
+/// whether any diagnostic occurred.
+static bool ProcessAddressSpaceAttribute(Sema &S, const ParsedAttr &Attr,

"Returns value" could be more specific. "Returns true if the attribute was 
processed successfully."


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57464/new/

https://reviews.llvm.org/D57464



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D57464: Generalize method overloading on addr spaces to C++

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: lib/Parse/ParseDeclCXX.cpp:2313
+}
+  }
 

Anastasia wrote:
> Anastasia wrote:
> > ebevhan wrote:
> > > Is there a reason that the attributes are parsed here and not in 
> > > `ParseFunctionDeclarator` like the rest of the ref-qualifiers (and the 
> > > OpenCL++ ASes)?
> > > 
> > > That is possibly why the parser is getting confused with the trailing 
> > > return.
> > Good question! I have a feeling that this was done to unify parsing of all 
> > CXX members, not just methods. For collecting the method qualifiers it 
> > would certainly help making flow more coherent if this was moved to 
> > `ParseFunctionDeclarator`. I will try to experiment with this a bit more. 
> > What I found strange that the attributes here are attached to the function 
> > type itself instead of its  qualifiers. May be @rjmccall can shed some more 
> > light on the overall flow...
> I looked at this a bit more and it seems that all the GNU attributes other 
> than addr spaces belong to the function (not to be applied to `this`) for 
> example 
> https://blog.timac.org/2016/0716-constructor-and-destructor-attributes/. It 
> seems correct to parse them here and apply to the function type. Although we 
> could separate parsing of addr space attribute only and move into 
> `ParseFunctionDeclarator`.  However this will only be needed for the function 
> type not for the data members. So not sure whether it will make the flow any 
> cleaner.
> I looked at this a bit more and it seems that all the GNU attributes other 
> than addr spaces belong to the function (not to be applied to this) 

Hm, I know what you mean. It's why Clang usually complains when you try placing 
an AS attribute after a function declarator, because it's trying to apply it to 
the function rather than the method qualifiers.

> However this will only be needed for the function type not for the data 
> members. 

But we aren't really interested in data members, are we? Like struct fields, 
non-static data members cannot be qualified with ASes (they should 'inherit' 
the AS from the object type), and static ones should probably just accept ASes 
as part of the member type itself.

These patches are to enable AS-qualifiers on methods, so it only stands to 
reason that those ASes would be parsed along with the other method qualifiers.

ParseFunctionDeclarator calls ParseTypeQualifierListOpt to get the 
cv-qualifier-seq for the method qualifiers. Currently it's set to 
`AR_NoAttributesParsed`. If we enable parsing attributes there, then the 
qualifier parsing might 'eat' attributes that were possibly meant for the 
function.

This is just a guess, but what I think you could do is include attributes in 
the cv-qualifier parsing with `AR_GNUAttributesParsed`, look for any AS 
attributes, take their AS and mark them invalid, then move the attributes 
(somehow) from the qualifier-DeclSpec to the `FnAttrs` function attribute list.

(The reason I'm concerned about where/how the qualifiers are parsed is because 
this approach only works for custom ASes applied with the GNU attribute 
directly. It won't work if custom address spaces are given with a custom 
keyword qualifier, like in OpenCL. If the ASes are parsed into the DeclSpec 
used for the other ref-qualifiers, then both of these cases should 'just work'.)



Comment at: test/SemaCXX/address-space-method-overloading.cpp:28
+   //inas4.bar();
+   noas.bar(); // expected-error{{call to member function 'bar' is ambiguous}}
+}

Just as an aside (I think I mentioned this on one of the earlier patches as 
well); treating a non-AS-qualified object as being in a 'generic' AS even for 
normal C++ isn't a great idea IMO. The object initialization code should be 
checking if the ASes of the actual object and the desired object type are 
compatible, and only if so, permit the overload.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57464/new/

https://reviews.llvm.org/D57464



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In https://reviews.llvm.org/D53738#1283459, @rjmccall wrote:

> Well, it could be passed around through most code as some sort of 
> abstractly-represented intermediate "type" which could be either a QualType 
> or a fixed-point semantics.


Sounds to me like you're describing a new `Type` that can contain an arbitrary 
fixed-point semantic :)

It still feels like this just introduces inconsistencies into the form of the 
AST. If we do add this extra type object to the BO, won't people wonder why we 
use ImplicitCastExpr for non-fixedpoint operations but use the special 
`QualTypeOrFPSemantics BinaryOperator::ComputationType;` for fixedpoint 
operations even though they both have nearly the same semantic meaning 
(converting to a common type before doing the operation)?

(The difference being that using the `ComputationType` requires you to cast 
back to the result type afterwards.)

> 
> 
>> It might just be easier to store the full-precision info in BO directly. BO 
>> might be too common to warrant the size increase, though. 
>> FixedPointSemantics can probably be optimized to only take 32 bits.
> 
> What you can definitely do is store a bit in BO saying that there's extra 
> storage for the intermediate "type".

Is this similar to how ExtQuals works? How would this be implemented?

>> As a side note, comparisons are still a bit up in the air. I don't think we 
>> came to a conclusion on whether they should be done in full precision or 
>> bitwise. The spec isn't clear.
> 
> ...bitwise?

The spec uses different wording for the arithmetic operations and comparison 
operations, and it's not entirely obvious what it means. For the arithmetic 
operators, it has the whole section on finding the full precision common type, 
but for comparisons it says:

> When comparing fixed-point values with fixed-point values or integer values,
>  the values are compared directly; the values of the operands are not 
> converted before the
>  comparison is made.

What 'directly' means in conjunction with 'the operands are not converted' is 
not clear. It's reasonable to assume that it either means comparing value-wise 
(so, the same as finding a common type that fits all values and then comparing; 
though this would obviously require conversion), or perhaps 'directly' means a 
bitwise (representation) comparison. The latter seems a bit farfetched to me, 
though.


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

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In https://reviews.llvm.org/D53738#1283983, @rjmccall wrote:

> In https://reviews.llvm.org/D53738#1283861, @ebevhan wrote:
>
> > In https://reviews.llvm.org/D53738#1283459, @rjmccall wrote:
> >
> > > Well, it could be passed around through most code as some sort of 
> > > abstractly-represented intermediate "type" which could be either a 
> > > QualType or a fixed-point semantics.
> >
> >
> > Sounds to me like you're describing a new `Type` that can contain an 
> > arbitrary fixed-point semantic :)
>
>
> The fact that it doesn't exist in the modeled type system is important.
>
> The arbitrary-type overflow intrinsics have this same problem, and we did not 
> try to solve it by creating a new type class for arbitrary-precision integers 
> and promoting the arguments.


Okay, I had a look at how those were emitted, I wasn't familiar with it until 
now. That's certainly a way of approaching this implementation as well, though 
I think the full precision info should still be in the AST somewhere rather 
than implied from the operand types until CodeGen.

> 
> 
>> It still feels like this just introduces inconsistencies into the form of 
>> the AST. If we do add this extra type object to the BO, won't people wonder 
>> why we use ImplicitCastExpr for non-fixedpoint operations but use the 
>> special `QualTypeOrFPSemantics BinaryOperator::ComputationType;` for 
>> fixedpoint operations even though they both have nearly the same semantic 
>> meaning (converting to a common type before doing the operation)?
> 
> 
> 
> 1. You would have an inconsistency in either case, since e.g. numeric + 
> otherwise always returns the same type as its operands, but this would not.

True, but the CompoundAssignOperator already has that inconsistency with 
ComputationResultType.

> 2. The question is easily answered by pointing at the language spec.  The 
> language does not say that the operands are promoted to a common type; it 
> says the result is determined numerically from the true numeric values of the 
> operands.

I guess. I just see that as a behavioral specification and not necessarily an 
implementation detail. It's perfectly acceptable to implement it as promotion 
to a common type (obviously, as that's what we are going to do), and I don't 
really see this as the spec putting any kind of requirement on how the 
implementation should be done.

> 
> 
 It might just be easier to store the full-precision info in BO directly. 
 BO might be too common to warrant the size increase, though. 
 FixedPointSemantics can probably be optimized to only take 32 bits.
>>> 
>>> What you can definitely do is store a bit in BO saying that there's extra 
>>> storage for the intermediate "type".
>> 
>> Is this similar to how ExtQuals works? How would this be implemented?
> 
> Take a look at how `DeclRefExpr` stores its various optional components.

`TrailingObjects`, then. That certainly wouldn't work if CompoundAssignOperator 
is still a subclass of BinaryOperator, so it would have to be folded in that 
case.

So the implementation would be with a `TrailingObjects` where it can have 2 QualTypes and 1 
FixedPointSemantics, the QualTypes being subsumed from CompoundAssignOperator.

Probably still quite a hefty amount of code that would have to be updated to 
make this change.

> 
> 
 As a side note, comparisons are still a bit up in the air. I don't think 
 we came to a conclusion on whether they should be done in full precision 
 or bitwise. The spec isn't clear.
>>> 
>>> ...bitwise?
>> 
>> The spec uses different wording for the arithmetic operations and comparison 
>> operations, and it's not entirely obvious what it means. For the arithmetic 
>> operators, it has the whole section on finding the full precision common 
>> type, but for comparisons it says:
>> 
>>> When comparing fixed-point values with fixed-point values or integer values,
>>>  the values are compared directly; the values of the operands are not 
>>> converted before the
>>>  comparison is made.
>> 
>> What 'directly' means in conjunction with 'the operands are not converted' 
>> is not clear. It's reasonable to assume that it either means comparing 
>> value-wise (so, the same as finding a common type that fits all values and 
>> then comparing; though this would obviously require conversion), or perhaps 
>> 'directly' means a bitwise (representation) comparison. The latter seems a 
>> bit farfetched to me, though.
> 
> I think this is just like fixed-point arithmetic: you should do an exact 
> numeric comparison, but there's no formal conversion because it would have to 
> be a conversion to some concrete type, which could leave to (mandatory!) 
> inexactness when there's no common type that expresses the full range of both 
> operands.

This is probably the correct interpretation, I just think it could have been 
stated a bit clearer that they pretty much do mean the same thing for the 
comparison operators as for th

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In https://reviews.llvm.org/D53738#1284213, @rjmccall wrote:

> >> 2. The question is easily answered by pointing at the language spec.  The 
> >> language does not say that the operands are promoted to a common type; it 
> >> says the result is determined numerically from the true numeric values of 
> >> the operands.
> > 
> > I guess. I just see that as a behavioral specification and not necessarily 
> > an implementation detail. It's perfectly acceptable to implement it as 
> > promotion to a common type (obviously, as that's what we are going to do), 
> > and I don't really see this as the spec putting any kind of requirement on 
> > how the implementation should be done.
>
> Of course it's a behavioral specification.  All I'm saying is that the 
> implementation detail of how we perform these operations isn't really 
> reasonable or appropriate to express in the AST.  I know it's a bit fuzzy 
> because of some of the things we do with e.g. opaque values and so on, but 
> the AST is not meant to be a completely implementation-defined IR; it tries 
> to capture the formal semantics of the program including "official" 
> conversions and so on.  Integer addition is specified as converting its 
> arguments to a common type and then performing a homogenous operation in that 
> type.  Fixed-point addition is not; it's specified as doing a heterogenous 
> operation on the original (well, rvalue-converted) operand types.


Okay, those are good points. I guess I might have been a bit too focused on 
reusing the behavior of the AST to fit the implementation, but that doesn't 
seem to be the intention with it.

> 
> 
>> It might just be easier to store the full-precision info in BO directly. 
>> BO might be too common to warrant the size increase, though. 
>> FixedPointSemantics can probably be optimized to only take 32 bits.
> 
> What you can definitely do is store a bit in BO saying that there's extra 
> storage for the intermediate "type".
 
 Is this similar to how ExtQuals works? How would this be implemented?
>>> 
>>> Take a look at how `DeclRefExpr` stores its various optional components.
>> 
>> `TrailingObjects`, then. That certainly wouldn't work if 
>> CompoundAssignOperator is still a subclass of BinaryOperator, so it would 
>> have to be folded in that case.
>> 
>> So the implementation would be with a `TrailingObjects> QualType, FixedPointSemantics>` where it can have 2 QualTypes and 1 
>> FixedPointSemantics, the QualTypes being subsumed from 
>> CompoundAssignOperator.
>> 
>> Probably still quite a hefty amount of code that would have to be updated to 
>> make this change.
> 
> Not out of line with other features that significantly break with what's 
> expressible.  But the easy alternative to storing the intermediate "type" in 
> the AST is to just provide a global function that can compute it on demand.

Yeah, it might just be simpler to go the route of not storing the computation 
semantic in the AST, at least for now. That's fairly similar to the solution in 
the patch, so I feel a bit silly for just going around in a circle.

To make that work I think the patch needs some changes, though. There should be 
a function in FixedPointSemantics to find the common full-precision semantic 
between two semantics, and the `getFixedPointSemantics` in ASTContext should be 
amended to take integer types (or another method should be provided for this, 
but that one already exists). I think that the `FixedPointConversions` method 
should also be embedded into the rest of `UsualArithmeticConversions` as there 
shouldn't be any need to have it separate. You still want to do the rvalue 
conversion and other promotions, and the rules for fixed-point still fit into 
the arithmetic conversions, even in the spec.

`EmitFixedPointConversion` should be changed to take FixedPointSemantics rather 
than QualType. This has a couple of downsides:

- It can no longer handle floating point conversions. They would have to be 
handled in EmitScalarConversion.
- Conversion from integer is fine, but conversion to integer cannot be 
generalized away with the fixed-point semantics as they are currently, as that 
kind of conversion must round towards zero. This requires a rounding step for 
negative numbers before downscaling, which the current code does not do.

Is there a better way of generalizing this?

All `EmitFixedPointAdd` should have to do is get the semantics of the operands 
and result type, get their full-precision semantic, call 
`EmitFixedPointConversion` on both operands, do the addition, and call it again 
to convert back to the result value. Move as much of the conversions as 
possible out of the function.

Does all this sound reasonable?


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

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In https://reviews.llvm.org/D53738#1288333, @rjmccall wrote:

> In https://reviews.llvm.org/D53738#1287123, @ebevhan wrote:
>
> > In https://reviews.llvm.org/D53738#1284213, @rjmccall wrote:
> >
> > > Not out of line with other features that significantly break with what's 
> > > expressible.  But the easy alternative to storing the intermediate "type" 
> > > in the AST is to just provide a global function that can compute it on 
> > > demand.
> >
> >
> > Yeah, it might just be simpler to go the route of not storing the 
> > computation semantic in the AST, at least for now. That's fairly similar to 
> > the solution in the patch, so I feel a bit silly for just going around in a 
> > circle.
> >
> > To make that work I think the patch needs some changes, though. There 
> > should be a function in FixedPointSemantics to find the common 
> > full-precision semantic between two semantics, and the 
> > `getFixedPointSemantics` in ASTContext should be amended to take integer 
> > types (or another method should be provided for this, but that one already 
> > exists). I think that the `FixedPointConversions` method should also be 
> > embedded into the rest of `UsualArithmeticConversions` as there shouldn't 
> > be any need to have it separate. You still want to do the rvalue conversion 
> > and other promotions, and the rules for fixed-point still fit into the 
> > arithmetic conversions, even in the spec.
> >
> > `EmitFixedPointConversion` should be changed to take FixedPointSemantics 
> > rather than QualType. This has a couple of downsides:
> >
> > - It can no longer handle floating point conversions. They would have to be 
> > handled in EmitScalarConversion.
> > - Conversion from integer is fine, but conversion to integer cannot be 
> > generalized away with the fixed-point semantics as they are currently, as 
> > that kind of conversion must round towards zero. This requires a rounding 
> > step for negative numbers before downscaling, which the current code does 
> > not do. Is there a better way of generalizing this?
>
>
> `FixedPointSemantics` is free to do whatever is convenient for the 
> representation; if it helps to make it able to explicitly represent an 
> integral type — and then just assert that it's never in that form when it's 
> used in certain places, I think that works.  Although you might consider 
> making a `FixedPointOrIntegralSemantics` class and then making 
> `FixedPointSemantics` a subclass which adds nothing to the representation but 
> semantically asserts that it's representing a fixed-point type.


It might just be simpler and a bit more general to add a 
`DownscalingRoundsTowardZero` field to `FixedPointSemantics`, which specifies 
that the source value should be rounded toward zero before downscaling. Then 
conversion routines could handle the integer case explicitly. I'm not sure if 
the field would be useful for anything else, though; it has a pretty specific 
meaning.

I think it's a bit odd to have `FixedPointSemantics` as a specialization of 
something else, since technically integers are a specialization of fixed-point 
values and not the other way around.


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

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

> The only thing I didn't include in this patch were the suggested changes to 
> FixedPointSemantics which would indicate if the semantics were original from 
> an integer type. I'm not sure how necessary this is because AFAIK the only 
> time we would need to care if the semantics came from an int is during 
> conversion from a fixed point type to an int, which should only occur during 
> casting and not binary operations. In that sense, I don't think this info 
> needs to be something bound to the semantics, but more to the conversion 
> operation. I also don't think that would be relevant for this patch since all 
> integers get converted to fixed point types anyway and not the other way 
> around.



> For the integer conversion though, I was going to add that in a separate 
> fixed-point-to-int and int-to-fixed-point patch.

Okay, that's fine! You're right in that the semantics commoning will never 
produce an integer semantic like that.




Comment at: clang/lib/Basic/FixedPoint.cpp:129
+  std::max(NonFractBits, OtherNonFractBits) + CommonScale;
+
+  bool ResultIsSigned = isSigned() || Other.isSigned();

Does this properly compensate for types of equal width but different signedness?

If you have a signed 8-bit 7-scale fixed point number, and operate with an 
unsigned 8-bit integer, you'll get CommonWidth=15 and CommonScale=7, leaving 8 
bits of integral. But the MSB in that cannot both be sign bit and unsigned MSB 
at the same time. I think you need an extra bit.



Comment at: clang/lib/Basic/FixedPoint.cpp:135
+ResultHasUnsignedPadding =
+hasUnsignedPadding() || Other.hasUnsignedPadding();
+

If one has padding but the other doesn't, then the padding must be significant, 
so the full precision semantic cannot have padding.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3385
+  if (IsCommonSaturated)
+CommonFixedSema.setSaturated(false);
+

Can EmitFixedPointConversion not determine this on its own?



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3387
+
+  // Convert and align the operands.
+  Value *LHSAligned = EmitFixedPointConversion(

'align' usually means something else. Maybe 'Convert the operands to the full 
precision type' and 'FullLHS'?



Comment at: clang/lib/Sema/SemaExpr.cpp:1306
+OtherTy = ResultTy;
+  }
+

Does this handle compound assignment? The other functions handle this specially.



Comment at: clang/lib/Sema/SemaExpr.cpp:1403
+  else if (RHSType->isFixedPointType())
+return handleFixedPointConversion(*this, RHS, LHS, RHSType, LHSType);
+

Can this commutation be folded into the function to align it with the rest?


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

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

I'd be interested in seeing tests for two saturating unsigned _Fract with and 
without padding.

If the padding case emits a uadd_sat, that seems wrong; uadd_sat doesn't 
saturate on the padding bit, but will saturate the whole number, which can 
result in invalid representation both with or without saturation taking effect.

Common semantics for unsigned types with padding might need a bit of trickery 
to get it to do the right thing.




Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3383
+
+  // onvert the operands to the full precision type.
+  Value *FullLHS = EmitFixedPointConversion(LHS, LHSFixedSema, CommonFixedSema,

Missing a C there.



Comment at: clang/lib/Sema/SemaExpr.cpp:1306
+OtherTy = ResultTy;
+  }
+

leonardchan wrote:
> ebevhan wrote:
> > Does this handle compound assignment? The other functions handle this 
> > specially.
> Currently no. I was initially intending to add addition compound assignment 
> in a different patch, but do you think it would be appropriate to include it 
> here? I imagine the changes to Sema wouldn't be difficult, but would probably 
> require adding a lot more tests. 
That's fine by me, then. Take it in the next patch.



Comment at: clang/test/Frontend/fixed_point_add.c:36
+
+  // To smaller scale and same width
+  // CHECK:  [[SA:%[0-9]+]] = load i16, i16* %sa, align 2

What do these comments refer to? The scale is the same for both operands 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

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3385
+  if (IsCommonSaturated)
+CommonFixedSema.setSaturated(false);
+

ebevhan wrote:
> Can EmitFixedPointConversion not determine this on its own?
What I meant here was that rather than zeroing out the saturation mode on the 
common semantic because we know that the common conversion won't saturate, 
EmitFixedPointConversion should be able to pick up on the fact that converting 
from semantic A to semantic B shouldn't cause saturation and not emit a 
saturating conversion in that case. Then you can set the saturation on the 
common semantic properly, since the operation should be saturating.

Even for something like `sat_uf * sat_uf` with padding, where the common type 
conversion should be `(16w, 15scale, uns, sat, pad) -> (15w, 15scale, uns, sat, 
nopad)`, EmitFixedPointConversion shouldn't emit a saturation, since the number 
of integral bits hasn't changed.



Comment at: clang/test/Frontend/fixed_point_add.c:269
+  // UNSIGNED-NEXT: [[SUM:%[0-9]+]] = call i15 @llvm.uadd.sat.i15(i15 
[[USA_TRUNC]], i15 [[USA_SAT_TRUNC]])
+  // UNSIGNED-NEXT: [[SUM_EXT:%[a-z0-9]+]] = zext i15 [[SUM]] to i16
+  // UNSIGNED-NEXT: store i16 [[SUM_EXT]], i16* %usa_sat, align 2

This is probably a candidate for an isel optimization. This operation also 
works as an `i16 ssat.add` with a negative-clamp-to-zero afterwards, and if the 
target supports `i16 ssat.add` natively then it will likely be a lot more 
efficient than whatever an `i15 uadd.sat` produces.


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

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In https://reviews.llvm.org/D53738#1305498, @leonardchan wrote:

> @ebevhan Any more comments on this patch?


It's strange, I feel like I've responded to the last comment twice now but 
there's nothing in Phab.

Generally I think it's good! One final note; I assume we could technically 
reuse/rename the EmitFixedPointAdd function and use it to emit other binops 
when those are added?




Comment at: clang/test/Frontend/fixed_point_add.c:269
+  // UNSIGNED-NEXT: [[SUM:%[0-9]+]] = call i15 @llvm.uadd.sat.i15(i15 
[[USA_TRUNC]], i15 [[USA_SAT_TRUNC]])
+  // UNSIGNED-NEXT: [[SUM_EXT:%[a-z0-9]+]] = zext i15 [[SUM]] to i16
+  // UNSIGNED-NEXT: store i16 [[SUM_EXT]], i16* %usa_sat, align 2

leonardchan wrote:
> ebevhan wrote:
> > This is probably a candidate for an isel optimization. This operation also 
> > works as an `i16 ssat.add` with a negative-clamp-to-zero afterwards, and if 
> > the target supports `i16 ssat.add` natively then it will likely be a lot 
> > more efficient than whatever an `i15 uadd.sat` produces.
> Do you think it would be more efficient for now then if instead we did SHL by 
> 1, saturate, then [AL]SHR by 1? This way we could use `i16 ssat.add` instead 
> of `i15 ssat.add`?
We should probably just do it in isel or instcombine instead. We don't know at 
this point which intrinsic is a better choice (though, I think 
power-of-two-types are generally better).


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

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In D53738#1308314 , @leonardchan wrote:

> > Generally I think it's good! One final note; I assume we could technically 
> > reuse/rename the EmitFixedPointAdd function and use it to emit other binops 
> > when those are added?
>
> Yes, but I imagine if we choose to keep the call to 
> `EmitFixedPointConversion` to cast both operands to a common type, this 
> wouldn't be reused for division or multiplication since I believe those do 
> not require for the operands to be converted to a common type.


They don't? The example given by the spec is even `int * _Fract`.




Comment at: clang/test/Frontend/fixed_point_add.c:269
+  // UNSIGNED-NEXT: [[SUM:%[0-9]+]] = call i15 @llvm.uadd.sat.i15(i15 
[[USA_TRUNC]], i15 [[USA_SAT_TRUNC]])
+  // UNSIGNED-NEXT: [[SUM_EXT:%[a-z0-9]+]] = zext i15 [[SUM]] to i16
+  // UNSIGNED-NEXT: store i16 [[SUM_EXT]], i16* %usa_sat, align 2

leonardchan wrote:
> ebevhan wrote:
> > leonardchan wrote:
> > > ebevhan wrote:
> > > > This is probably a candidate for an isel optimization. This operation 
> > > > also works as an `i16 ssat.add` with a negative-clamp-to-zero 
> > > > afterwards, and if the target supports `i16 ssat.add` natively then it 
> > > > will likely be a lot more efficient than whatever an `i15 uadd.sat` 
> > > > produces.
> > > Do you think it would be more efficient for now then if instead we did 
> > > SHL by 1, saturate, then [AL]SHR by 1? This way we could use `i16 
> > > ssat.add` instead of `i15 ssat.add`?
> > We should probably just do it in isel or instcombine instead. We don't know 
> > at this point which intrinsic is a better choice (though, I think 
> > power-of-two-types are generally better).
> Ok. Are you suggesting something should be changed here though? I imagine 
> that during legalization, `i15 ssat.add` would be legalized into `i16 
> ssat.add` if that is what's natively supported.
No, it doesn't have to be changed. Just something to keep in mind.
> i15 ssat.add would be legalized into i16 ssat.add if that is what's natively 
> supported.
Sure, but I meant that `i15 usat.add` could be more efficient as `i16 ssat.add`.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53738/new/

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In D53738#1310212 , @leonardchan wrote:

> In D53738#1309171 , @ebevhan wrote:
>
> > In D53738#1308314 , @leonardchan 
> > wrote:
> >
> > > > Generally I think it's good! One final note; I assume we could 
> > > > technically reuse/rename the EmitFixedPointAdd function and use it to 
> > > > emit other binops when those are added?
> > >
> > > Yes, but I imagine if we choose to keep the call to 
> > > `EmitFixedPointConversion` to cast both operands to a common type, this 
> > > wouldn't be reused for division or multiplication since I believe those 
> > > do not require for the operands to be converted to a common type.
> >
> >
> > They don't? The example given by the spec is even `int * _Fract`.
>
>
> Oh, I meant that the scales of both operands won't need to be aligned before 
> performing the operation. Since for multiplication, we can multiply fixed 
> point numbers in any scale without shifting and only need to perform a shift 
> on the result to convert to the destination type. Although this would only 
> apply to non-saturating multiplication since to use the intrinsics, both 
> operands would need to be in the same scale.


I see what you mean, but the fixed-point multiplication intrinsic requires the 
operands to be in the same scale.

It's certainly interesting to degenerate integer-with-fixedpoint to just a mul 
(since the scaling factor is 2^-n * 2^0, which is just 2^-n), but in the 
general case you can't avoid doing the scale alignment. Unless I'm missing 
something.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53738/new/

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

rjmccall wrote:
> Hmm.  So adding a signed integer to an unsigned fixed-point type always 
> converts the integer to unsigned?  That's a little weird, but only because 
> the fixed-point rule seems to be the other way.  Anyway, I assume it's not a 
> bug in the spec.
`handleFixedPointConversion` only calculates the result type of the expression, 
not the calculation type. The final result type of the operation will be the 
unsigned fixed-point type, but the calculation itself will be done in a signed 
type with enough precision to represent both the signed integer and the 
unsigned fixed-point type. 

Though, if the result ends up being negative, the final result is undefined 
unless the type is saturating. I don't think there is a test for the saturating 
case (signed integer + unsigned saturating fixed-point) in the 
SaturatedAddition tests.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53738/new/

https://reviews.llvm.org/D53738



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan updated this revision to Diff 176130.
ebevhan added a comment.

Rebased and addressed review comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51211/new/

https://reviews.llvm.org/D51211

Files:
  lib/Sema/SemaChecking.cpp
  test/Sema/format-strings-bitfield-promotion.c


Index: test/Sema/format-strings-bitfield-promotion.c
===
--- /dev/null
+++ test/Sema/format-strings-bitfield-promotion.c
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+
+int printf(const char *restrict, ...);
+
+struct bitfields {
+  long a : 2;
+  unsigned long b : 2;
+  long c : 32;  // assumes that int is 32 bits
+  unsigned long d : 32; // assumes that int is 32 bits
+} bf;
+
+void bitfield_promotion() {
+  printf("%ld", bf.a); // expected-warning {{format specifies type 'long' but 
the argument has type 'int'}}
+  printf("%lu", bf.b); // expected-warning {{format specifies type 'unsigned 
long' but the argument has type 'int'}}
+  printf("%ld", bf.c); // expected-warning {{format specifies type 'long' but 
the argument has type 'int'}}
+  printf("%lu", bf.d); // expected-warning {{format specifies type 'unsigned 
long' but the argument has type 'unsigned int'}}
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -7697,6 +7697,24 @@
   return std::make_pair(QualType(), StringRef());
 }
 
+/// Return true if \p ICE is an implicit argument promotion of an arithmetic
+/// type. Bit-field 'promotions' from a higher ranked type to a lower ranked
+/// type do not count.
+static bool
+isArithmeticArgumentPromotion(Sema &S, const ImplicitCastExpr *ICE) {
+  QualType From = ICE->getSubExpr()->getType();
+  QualType To = ICE->getType();
+  // It's a floating promotion if the source type is a lower rank.
+  if (ICE->getCastKind() == CK_FloatingCast &&
+  S.Context.getFloatingTypeOrder(From, To) < 0)
+return true;
+  // It's an integer promotion if the destination type is the promoted
+  // source type.
+  return ICE->getCastKind() == CK_IntegralCast &&
+ From->isPromotableIntegerType() &&
+ S.Context.getPromotedIntegerType(From) == To;
+}
+
 bool
 CheckPrintfHandler::checkFormatExpr(const analyze_printf::PrintfSpecifier &FS,
 const char *StartSpecifier,
@@ -7724,11 +7742,11 @@
 
   // Look through argument promotions for our error message's reported type.
   // This includes the integral and floating promotions, but excludes array
-  // and function pointer decay; seeing that an argument intended to be a
-  // string has type 'char [6]' is probably more confusing than 'char *'.
+  // and function pointer decay (seeing that an argument intended to be a
+  // string has type 'char [6]' is probably more confusing than 'char *') and
+  // certain bitfield promotions (bitfields can be 'demoted' to a lesser type).
   if (const ImplicitCastExpr *ICE = dyn_cast(E)) {
-if (ICE->getCastKind() == CK_IntegralCast ||
-ICE->getCastKind() == CK_FloatingCast) {
+if (isArithmeticArgumentPromotion(S, ICE)) {
   E = ICE->getSubExpr();
   ExprTy = E->getType();
 


Index: test/Sema/format-strings-bitfield-promotion.c
===
--- /dev/null
+++ test/Sema/format-strings-bitfield-promotion.c
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+
+int printf(const char *restrict, ...);
+
+struct bitfields {
+  long a : 2;
+  unsigned long b : 2;
+  long c : 32;  // assumes that int is 32 bits
+  unsigned long d : 32; // assumes that int is 32 bits
+} bf;
+
+void bitfield_promotion() {
+  printf("%ld", bf.a); // expected-warning {{format specifies type 'long' but the argument has type 'int'}}
+  printf("%lu", bf.b); // expected-warning {{format specifies type 'unsigned long' but the argument has type 'int'}}
+  printf("%ld", bf.c); // expected-warning {{format specifies type 'long' but the argument has type 'int'}}
+  printf("%lu", bf.d); // expected-warning {{format specifies type 'unsigned long' but the argument has type 'unsigned int'}}
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -7697,6 +7697,24 @@
   return std::make_pair(QualType(), StringRef());
 }
 
+/// Return true if \p ICE is an implicit argument promotion of an arithmetic
+/// type. Bit-field 'promotions' from a higher ranked type to a lower ranked
+/// type do not count.
+static bool
+isArithmeticArgumentPromotion(Sema &S, const ImplicitCastExpr *ICE) {
+  QualType From = ICE->getSubExpr()->getType();
+  QualType To = ICE->getType();
+  // It's a floating promotion if the source type is a lower rank.
+  if (ICE->getCastKind() == CK_FloatingCast &

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan marked an inline comment as done.
ebevhan added inline comments.



Comment at: lib/Sema/SemaChecking.cpp:7711-7715
+  // It's an integer promotion if the destination type is the promoted
+  // source type.
+  return ICE->getCastKind() == CK_IntegralCast &&
+ From->isPromotableIntegerType() &&
+ S.Context.getPromotedIntegerType(From) == To;

aaron.ballman wrote:
> This check is not checking against the promoted type of the bit-field. See 
> `Sema::UsualArithmeticConversions()` for an example of what I'm talking 
> about. Is that expected?
I'm not entirely sure what you mean. Are you referring to the type produced by 
`isPromotableBitField`? The source type of that promotion is what we don't want 
to see in these implicit casts.

We don't want to look through promotions here if we promoted from a type which 
was the result of a bitfield promotion, and that bitfield promotion was from a 
higher ranked type to a lower ranked type. so, if we have a bitfield of type 
`short`, then promoting that to `int` is fine, and we will give a warning for 
the `short`. But if the type of the bitfield is `long`, it could be promoted to 
`int`. However, the format specifier warning will look through these promotions 
and think that we passed an expression of `long` to the function even though it 
was `int`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51211/new/

https://reviews.llvm.org/D51211



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:1304
+RHSTy = ResultTy;
+  }
+

rjmccall wrote:
> leonardchan wrote:
> > rjmccall wrote:
> > > ebevhan wrote:
> > > > rjmccall wrote:
> > > > > Hmm.  So adding a signed integer to an unsigned fixed-point type 
> > > > > always converts the integer to unsigned?  That's a little weird, but 
> > > > > only because the fixed-point rule seems to be the other way.  Anyway, 
> > > > > I assume it's not a bug in the spec.
> > > > `handleFixedPointConversion` only calculates the result type of the 
> > > > expression, not the calculation type. The final result type of the 
> > > > operation will be the unsigned fixed-point type, but the calculation 
> > > > itself will be done in a signed type with enough precision to represent 
> > > > both the signed integer and the unsigned fixed-point type. 
> > > > 
> > > > Though, if the result ends up being negative, the final result is 
> > > > undefined unless the type is saturating. I don't think there is a test 
> > > > for the saturating case (signed integer + unsigned saturating 
> > > > fixed-point) in the SaturatedAddition tests.
> > > > `handleFixedPointConversion` only calculates the result type of the 
> > > > expression, not the calculation type.
> > > 
> > > Right, I understand that, but the result type of the expression obviously 
> > > impacts the expressive range of the result, since you can end up with a 
> > > negative value.
> > > 
> > > Hmm.  With that said, if the general principle is to perform the 
> > > operation with full precision on the original operand values, why are 
> > > unsigned fixed-point operands coerced to their corresponding signed types 
> > > *before* the operation?  This is precision-limiting if the unsigned 
> > > representation doesn't use a padding bit.  That seems like a bug in the 
> > > spec.
> > > Hmm. With that said, if the general principle is to perform the operation 
> > > with full precision on the original operand values, why are unsigned 
> > > fixed-point operands coerced to their corresponding signed types *before* 
> > > the operation? This is precision-limiting if the unsigned representation 
> > > doesn't use a padding bit. That seems like a bug in the spec.
> > 
> > Possibly. When the standard mentions converting from signed to unsigned 
> > fixed point, the only requirement involved is conservation of magnitude 
> > (the number of integral bits excluding the sign)
> > 
> > ```
> > when signed and unsigned fixed-point types are mixed, the unsigned type is 
> > converted to the corresponding signed type, and this should go without loss 
> > of magnitude
> > ```
> > 
> > This is just speculation, but I'm under the impression that not as much 
> > "attention" was given for unsigned types as for signed types since "`In the 
> > embedded processor world, support for unsigned fixed-point data types is 
> > rare; normally only signed fixed-point data types are supported`", but was 
> > kept anyway since unsigned types are used a lot.
> > 
> > ```
> > However, to disallow unsigned fixed-point arithmetic from programming 
> > languages (in general, and from C in particular) based on this observation, 
> > seems overly restrictive.
> > ```
> > 
> > I also imagine that if the programmer needs more precision, the correct 
> > approach would be to cast up to a type with a higher scale. The standard 
> > seems to make an effort to expose as much in regards to the underlying 
> > fixed point types as possible:
> > 
> > ```
> > it should be possible to write fixed-point algorithms that are independent 
> > of the actual fixed-point hardware support. This implies that a programmer 
> > (or a running program) should have access to all parameters that define the 
> > behavior of the underlying hardware (in other words: even if these 
> > parameters are implementation-defined).
> > ```
> > 
> > So the user would at least know that unsigned types may not have the same 
> > scale as their corresponding signed types if the hardware handles them with 
> > different scales.
> > 
> > Also added test for signed integer + unsigned saturating fixed point.
> As long as we maintain the same typing behavior, does the standard permit us 
> to just Do The Right Thing here and do the extended arithmetic with the 
> original unsigned operand?  I'm sure there are some cases where we would 
> produce a slightly different value than an implementation that does the 
> coercion before the operation, but that might be permitted under the 
> standard, and as you say, it would only affect some situations that it 
> doesn't seem the standard has given much thought to.
The coercion of unsigned to signed is likely done for pragmatic reasons; if you 
have a signed and unsigned type of the same rank, operating on them together 
won't require any 'extra bits'. This means that if your hardware has native 
support for the operations, you won't end up in a si

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan marked 2 inline comments as done.
ebevhan added inline comments.



Comment at: test/Sema/format-strings-bitfield-promotion.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+

aaron.ballman wrote:
> aaron.ballman wrote:
> > Running your test through GCC looks like the behavior matches here for C; 
> > can you also add a C++ test that demonstrates the behavior does not change?
> > 
> > https://godbolt.org/z/zRYDMG
> Strangely, the above godbolt link dropped the output windows, here's a 
> different link that shows the behavioral differences between C and C++ mode 
> in GCC: https://godbolt.org/z/R3zRHe
Hmm, I'll have a look at this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51211/new/

https://reviews.llvm.org/D51211



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan marked an inline comment as done.
ebevhan added inline comments.



Comment at: test/Sema/format-strings-bitfield-promotion.c:1
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
+

ebevhan wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > Running your test through GCC looks like the behavior matches here for C; 
> > > can you also add a C++ test that demonstrates the behavior does not 
> > > change?
> > > 
> > > https://godbolt.org/z/zRYDMG
> > Strangely, the above godbolt link dropped the output windows, here's a 
> > different link that shows the behavioral differences between C and C++ mode 
> > in GCC: https://godbolt.org/z/R3zRHe
> Hmm, I'll have a look at this.
That gcc godbolt is a bit odd. The type of the bitfield expression in the C++ 
example is `long` and not `int`, but in Clang, it's clearly being converted. If 
I change the example a bit, we get this warning:
```
:11:12: warning: format '%d' expects argument of type 'int', but 
argument 2 has type 'long int' [-Wformat=]
   11 |   printf("%d", bf.a); // expected-warning {{format specifies type 
'long' but the argument has type 'int'}}
  |   ~^   
  ||  |
  |intlong int
```
But in Clang, we get a cast to `int`:
```
| `-ImplicitCastExpr 0xd190748  'int' 
|   `-ImplicitCastExpr 0xd190730  'long' 
| `-MemberExpr 0xd190618  'long' lvalue bitfield .a 
0xd18f790
|   `-DeclRefExpr 0xd1905f8  'struct bitfields':'bitfields' 
lvalue Var 0xd18fa18 'bf' 'struct bitfields':'bitfields'
```

So gcc and Clang are doing things differently here.

The code in `isPromotableBitField` says:
```
  // FIXME: C does not permit promotion of a 'long : 3' bitfield to int.
  //We perform that promotion here to match GCC and C++.
```
but clearly gcc isn't doing this in the C++ case. The comments also mention 
some things about gcc bugs that Clang does not follow, but that's in reference 
to a C DR.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51211/new/

https://reviews.llvm.org/D51211



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits