[PATCH] D43590: [clang-format] Fix regression when getStyle() called with empty filename

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: unittests/Format/FormatTest.cpp:11727
+TEST(FormatStyle, GetStyleWithEmptyFileName) {
+  auto Style1 = getStyle("file", "", "Google");
+  ASSERT_TRUE((bool)Style1);

Do you really want to try to find a ".clang-format" file here, with fallback to 
"Google" if no such file is found?

When I'm building I usually end up having my build directory inside the llvm 
repo. And since there is a .clang-format file checked in to llvm that file is 
found, as it searches for a .clang-format file somewhere in the directory 
structure above the current dir when running the test (if I remember 
correctly?). We have had such problem before.

Can't you just as well do
  auto Style1 = getStyle("Google", "", "Google");
or is that not triggering the original bug?

Right now our build bots ends up like this (I guess it has found the 
.clang-format in my llvm/clang repo and decided to use "LLVM" as format for 
"Style1"):

```
FAIL: Clang-Unit :: Format/./FormatTests/FormatStyle.GetStyleWithEmptyFileName 
(14009 of 36611)
 TEST 'Clang-Unit :: 
Format/./FormatTests/FormatStyle.GetStyleWithEmptyFileName' FAILED 

Note: Google Test filter = FormatStyle.GetStyleWithEmptyFileName
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from FormatStyle
[ RUN  ] FormatStyle.GetStyleWithEmptyFileName
../tools/clang/unittests/Format/FormatTest.cpp:11729: Failure
  Expected: *Style1
  Which is: 456-byte object 
To be equal to: getGoogleStyle()
  Which is: 456-byte object 
[  FAILED  ] FormatStyle.GetStyleWithEmptyFileName (1 ms)
[--] 1 test from FormatStyle (1 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (1 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] FormatStyle.GetStyleWithEmptyFileName

```



Repository:
  rC Clang

https://reviews.llvm.org/D43590



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

[PATCH] D43732: Resolve build bot problems in unittests/Format/FormatTest.cpp

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope created this revision.
Herald added a subscriber: klimek.

Make the new GetStyleWithEmptyFileName test case independent
of the file system used when running the test. Since the
test is supposed to use the fallback "Google" style we now
use a InMemoryFileSystem to make sure that we do not accidentaly
find a .clang-format file in the real file system. That could
for example happen when having the build directory inside the
llvm och clang repo (as there is a .clang-format file inside
the repos).


Repository:
  rC Clang

https://reviews.llvm.org/D43732

Files:
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11724,7 +11724,8 @@
 }
 
 TEST(FormatStyle, GetStyleWithEmptyFileName) {
-  auto Style1 = getStyle("file", "", "Google");
+  vfs::InMemoryFileSystem FS;
+  auto Style1 = getStyle("file", "", "Google", "", &FS);
   ASSERT_TRUE((bool)Style1);
   ASSERT_EQ(*Style1, getGoogleStyle());
 }


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11724,7 +11724,8 @@
 }
 
 TEST(FormatStyle, GetStyleWithEmptyFileName) {
-  auto Style1 = getStyle("file", "", "Google");
+  vfs::InMemoryFileSystem FS;
+  auto Style1 = getStyle("file", "", "Google", "", &FS);
   ASSERT_TRUE((bool)Style1);
   ASSERT_EQ(*Style1, getGoogleStyle());
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43590: [clang-format] Fix regression when getStyle() called with empty filename

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: unittests/Format/FormatTest.cpp:11727
+TEST(FormatStyle, GetStyleWithEmptyFileName) {
+  auto Style1 = getStyle("file", "", "Google");
+  ASSERT_TRUE((bool)Style1);

bjope wrote:
> Do you really want to try to find a ".clang-format" file here, with fallback 
> to "Google" if no such file is found?
> 
> When I'm building I usually end up having my build directory inside the llvm 
> repo. And since there is a .clang-format file checked in to llvm that file is 
> found, as it searches for a .clang-format file somewhere in the directory 
> structure above the current dir when running the test (if I remember 
> correctly?). We have had such problem before.
> 
> Can't you just as well do
>   auto Style1 = getStyle("Google", "", "Google");
> or is that not triggering the original bug?
> 
> Right now our build bots ends up like this (I guess it has found the 
> .clang-format in my llvm/clang repo and decided to use "LLVM" as format for 
> "Style1"):
> 
> ```
> FAIL: Clang-Unit :: 
> Format/./FormatTests/FormatStyle.GetStyleWithEmptyFileName (14009 of 36611)
>  TEST 'Clang-Unit :: 
> Format/./FormatTests/FormatStyle.GetStyleWithEmptyFileName' FAILED 
> 
> Note: Google Test filter = FormatStyle.GetStyleWithEmptyFileName
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from FormatStyle
> [ RUN  ] FormatStyle.GetStyleWithEmptyFileName
> ../tools/clang/unittests/Format/FormatTest.cpp:11729: Failure
>   Expected: *Style1
>   Which is: 456-byte object  00-00 01-01 01-00 00-00 00-00 04-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 
> 00-00 01-01 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 ... 
> 00-00 00-00 00-00 00-00 01-01 01-00 01-01 6E-53 01-00 00-00 00-00 00-00 01-00 
> 00-00 00-01 00-00 00-00 00-00 01-00 00-00 08-00 00-00 00-00 00-00 10-73 77-01 
> 00-00 00-00 50-73 77-01 00-00 00-00>
> To be equal to: getGoogleStyle()
>   Which is: 456-byte object  00-00 01-01 01-00 00-00 00-00 04-00 00-00 01-01 00-00 00-00 00-00 00-00 00-00 
> 01-01 01-01 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00 ... 
> 38-4A 77-01 00-00 00-00 01-01 01-00 01-01 00-00 01-00 00-00 00-00 00-00 02-00 
> 00-00 00-01 00-00 00-73 77-01 02-00 00-00 08-00 00-00 00-00 00-00 00-00 00-00 
> 00-00 00-00 00-00 00-00 00-00 00-00>
> [  FAILED  ] FormatStyle.GetStyleWithEmptyFileName (1 ms)
> [--] 1 test from FormatStyle (1 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (1 ms total)
> [  PASSED  ] 0 tests.
> [  FAILED  ] 1 test, listed below:
> [  FAILED  ] FormatStyle.GetStyleWithEmptyFileName
> 
> ```
> 
I've proposed a fix here: https://reviews.llvm.org/D43732
By using an InMemoryFileSystem to avoid relying on no .clang-format present in 
the path of the real file system when running the test.


Repository:
  rC Clang

https://reviews.llvm.org/D43590



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

[PATCH] D43732: Resolve build bot problems in unittests/Format/FormatTest.cpp

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

This is an attempt to fix problems seen after https://reviews.llvm.org/D43590


Repository:
  rC Clang

https://reviews.llvm.org/D43732



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

[PATCH] D43732: Resolve build bot problems in unittests/Format/FormatTest.cpp

[Bjorn Pettersson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC326086: Resolve build bot problems in 
unittests/Format/FormatTest.cpp (authored by bjope, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D43732?vs=135794&id=135891#toc

Repository:
  rC Clang

https://reviews.llvm.org/D43732

Files:
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11724,7 +11724,8 @@
 }
 
 TEST(FormatStyle, GetStyleWithEmptyFileName) {
-  auto Style1 = getStyle("file", "", "Google");
+  vfs::InMemoryFileSystem FS;
+  auto Style1 = getStyle("file", "", "Google", "", &FS);
   ASSERT_TRUE((bool)Style1);
   ASSERT_EQ(*Style1, getGoogleStyle());
 }


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -11724,7 +11724,8 @@
 }
 
 TEST(FormatStyle, GetStyleWithEmptyFileName) {
-  auto Style1 = getStyle("file", "", "Google");
+  vfs::InMemoryFileSystem FS;
+  auto Style1 = getStyle("file", "", "Google", "", &FS);
   ASSERT_TRUE((bool)Style1);
   ASSERT_EQ(*Style1, getGoogleStyle());
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43732: Resolve build bot problems in unittests/Format/FormatTest.cpp

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

In https://reviews.llvm.org/D43732#1020639, @benhamilton wrote:

> Actually, looks like all the other tests explicitly want a non-empty 
> in-memory FS, so there's no good cleanup I can do.


Well, it was only this new test case that failed, so I think other tests are 
fine (some of them have been adjusted in a similar way in the past if I 
remember correctly).
And our downstream integration bots has started to pull in things from trunk 
again, after I landed this fix.


Repository:
  rC Clang

https://reviews.llvm.org/D43732



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

[PATCH] D53299: [Fixed Point Arithmetic] Fix for clang-tools-extra warning

[Bjorn Pettersson via Phabricator via cfe-commits]

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

Just some inline nit:s about whitespace.

LGTM, apart from that!




Comment at: 
clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp:63
 InitType->getAs()->getElementType());
+  case Type::STK_FixedPoint:
+switch (InitType->getAs()->getKind()) {

nit: I'd add an extra line break before this line (just to follow the earlier 
style where it seems like cases are put in different groups in this switch).



Comment at: 
clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp:65
+switch (InitType->getAs()->getKind()) {
+  case BuiltinType::ShortAccum:
+  case BuiltinType::SatShortAccum:

Indents: I think case should be aligned with the switch.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53299



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

[PATCH] D53707: [Fixed Point Arithmetic] Refactor fixed point casts

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope created this revision.
bjope added reviewers: leonardchan, ebevhan.

- Added names for some emitted values (such as "tobool" for the result of a 
cast to boolean).
- Replaced explicit IRBuilder request for doing sext/zext/trunc by using 
CreateIntCast instead.
- Simplify code for emitting satuation into one if-statement for clamping to 
max, and one if-statement for clamping to min.


Repository:
  rC Clang

https://reviews.llvm.org/D53707

Files:
  lib/CodeGen/CGExprScalar.cpp
  test/Frontend/fixed_point_conversions.c
  test/Frontend/fixed_point_to_bool.c

Index: test/Frontend/fixed_point_to_bool.c
===
--- test/Frontend/fixed_point_to_bool.c
+++ test/Frontend/fixed_point_to_bool.c
@@ -27,26 +27,26 @@
   b = (_Bool)0.5ur;
   b = (_Bool)0.0ur;
 
-  // CHECK-NEXT: [[ACCUM:%[0-9]+]] = load i32, i32* %a, align 4
-  // CHECK-NEXT: [[NOTZERO:%[0-9]+]] = icmp ne i32 [[ACCUM]], 0
-  // CHECK-NEXT: %frombool = zext i1 [[NOTZERO]] to i8
-  // CHECK-NEXT: store i8 %frombool, i8* %b, align 1
+  // CHECK-NEXT: [[ACCUM:%[0-9a-z]+]] = load i32, i32* %a, align 4
+  // CHECK-NEXT: [[NOTZERO:%[0-9a-z]+]] = icmp ne i32 [[ACCUM]], 0
+  // CHECK-NEXT: [[FROMBOOL:%[0-9a-z]+]] = zext i1 [[NOTZERO]] to i8
+  // CHECK-NEXT: store i8 [[FROMBOOL]], i8* %b, align 1
   b = a;
 
-  // CHECK-NEXT: [[ACCUM:%[0-9]+]] = load i32, i32* %ua, align 4
-  // CHECK-NEXT: [[NOTZERO:%[0-9]+]] = icmp ne i32 [[ACCUM]], 0
-  // CHECK-NEXT: %frombool1 = zext i1 [[NOTZERO]] to i8
-  // CHECK-NEXT: store i8 %frombool1, i8* %b, align 1
+  // CHECK-NEXT: [[ACCUM:%[0-9a-z]+]] = load i32, i32* %ua, align 4
+  // CHECK-NEXT: [[NOTZERO:%[0-9a-z]+]] = icmp ne i32 [[ACCUM]], 0
+  // CHECK-NEXT: [[FROMBOOL:%[0-9a-z]+]] = zext i1 [[NOTZERO]] to i8
+  // CHECK-NEXT: store i8 [[FROMBOOL]], i8* %b, align 1
   b = ua;
 
-  // CHECK-NEXT: [[ACCUM:%[0-9]+]] = load i32, i32* %a, align 4
-  // CHECK-NEXT: [[NOTZERO:%[0-9]+]] = icmp ne i32 [[ACCUM]], 0
+  // CHECK-NEXT: [[ACCUM:%[0-9a-z]+]] = load i32, i32* %a, align 4
+  // CHECK-NEXT: [[NOTZERO:%[0-9a-z]+]] = icmp ne i32 [[ACCUM]], 0
   // CHECK-NEXT: br i1 [[NOTZERO]], label %if.then, label %if.end
   if (a) {
   }
 
-  // CHECK:  [[ACCUM:%[0-9]+]] = load i32, i32* %ua, align 4
-  // CHECK-NEXT: [[NOTZERO:%[0-9]+]] = icmp ne i32 [[ACCUM]], 0
+  // CHECK:  [[ACCUM:%[0-9a-z]+]] = load i32, i32* %ua, align 4
+  // CHECK-NEXT: [[NOTZERO:%[0-9a-z]+]] = icmp ne i32 [[ACCUM]], 0
   // CHECK-NEXT: br i1 [[NOTZERO]], label %if.then{{[0-9]+}}, label %if.end{{[0-9]+}}
   if (ua) {
   }
Index: test/Frontend/fixed_point_conversions.c
===
--- test/Frontend/fixed_point_conversions.c
+++ test/Frontend/fixed_point_conversions.c
@@ -4,104 +4,104 @@
 void TestFixedPointCastSameType() {
   _Accum a = 2.5k;
   _Accum a2 = a;
-  // DEFAULT:  [[ACCUM:%[0-9]+]] = load i32, i32* %a, align 4
+  // DEFAULT:  [[ACCUM:%[0-9a-z]+]] = load i32, i32* %a, align 4
   // DEFAULT-NEXT: store i32 [[ACCUM]], i32* %a2, align 4
 
   a2 = (_Accum)a;
-  // DEFAULT:  [[ACCUM:%[0-9]+]] = load i32, i32* %a, align 4
+  // DEFAULT:  [[ACCUM:%[0-9a-z]+]] = load i32, i32* %a, align 4
   // DEFAULT-NEXT: store i32 [[ACCUM]], i32* %a2, align 4
 }
 
 void TestFixedPointCastDown() {
   long _Accum la = 2.5lk;
   _Accum a = la;
-  // DEFAULT:  [[LACCUM:%[0-9]+]] = load i64, i64* %la, align 8
-  // DEFAULT-NEXT: [[ACCUM_AS_I64:%[0-9]+]] = ashr i64 [[LACCUM]], 16
-  // DEFAULT-NEXT: [[ACCUM:%[0-9]+]] = trunc i64 [[ACCUM_AS_I64]] to i32
+  // DEFAULT:  [[LACCUM:%[0-9a-z]+]] = load i64, i64* %la, align 8
+  // DEFAULT-NEXT: [[ACCUM_AS_I64:%[0-9a-z]+]] = ashr i64 [[LACCUM]], 16
+  // DEFAULT-NEXT: [[ACCUM:%[0-9a-z]+]] = trunc i64 [[ACCUM_AS_I64]] to i32
   // DEFAULT-NEXT: store i32 [[ACCUM]], i32* %a, align 4
 
   a = (_Accum)la;
-  // DEFAULT:  [[LACCUM:%[0-9]+]] = load i64, i64* %la, align 8
-  // DEFAULT-NEXT: [[ACCUM_AS_I64:%[0-9]+]] = ashr i64 [[LACCUM]], 16
-  // DEFAULT-NEXT: [[ACCUM:%[0-9]+]] = trunc i64 [[ACCUM_AS_I64]] to i32
+  // DEFAULT:  [[LACCUM:%[0-9a-z]+]] = load i64, i64* %la, align 8
+  // DEFAULT-NEXT: [[ACCUM_AS_I64:%[0-9a-z]+]] = ashr i64 [[LACCUM]], 16
+  // DEFAULT-NEXT: [[ACCUM:%[0-9a-z]+]] = trunc i64 [[ACCUM_AS_I64]] to i32
   // DEFAULT-NEXT: store i32 [[ACCUM]], i32* %a, align 4
 
   short _Accum sa = a;
-  // DEFAULT:  [[ACCUM:%[0-9]+]] = load i32, i32* %a, align 4
-  // DEFAULT-NEXT: [[SACCUM_AS_I32:%[0-9]+]] = ashr i32 [[ACCUM]], 8
-  // DEFAULT-NEXT: [[SACCUM:%[0-9]+]] = trunc i32 [[SACCUM_AS_I32]] to i16
+  // DEFAULT:  [[ACCUM:%[0-9a-z]+]] = load i32, i32* %a, align 4
+  // DEFAULT-NEXT: [[SACCUM_AS_I32:%[0-9a-z]+]] = ashr i32 [[ACCUM]], 8
+  // DEFAULT-NEXT: [[SACCUM:%[0-9a-z]+]] = trunc i32 [[SACCUM_AS_I32]] to i16
   // DEFAULT-NEXT: store i16 [[SACCUM]], i16* %sa, align 2
 
   sa = (short _Accum)a;
-  // DEFAULT:  [[ACCUM:%[0-9]+]] = load i32, i32* %a, al

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

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3208
+  if (LHSWidth < CommonWidth)
+LHS = LHSSign ? Builder.CreateSExt(LHS, CommonTy)
+  : Builder.CreateZExt(LHS, CommonTy);

`LHS = Builder.CreateIntCast(LHS, CommonTy, LHSSign);`

(I think that it even is possible to remove the condition and always do this 
(the cast will be a no-op if LHSWidth==CommonWidth))



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3211
+  if (RHSWidth < CommonWidth)
+RHS = RHSSign ? Builder.CreateSExt(RHS, CommonTy)
+  : Builder.CreateZExt(RHS, CommonTy);

Same as above, this can be simplified as:
  RHS = Builder.CreateIntCast(RHS, CommonTy, RHSSign);




Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3214
+
+  // Align scales
+  unsigned CommonScale = std::max({LHSScale, RHSScale, ResultScale});

Arithmetically I think that it would be enough to align the scale to
```
 std::max(std::min(LHSScale, RHSScale), ResultScale)
```
As adding low fractional bits outside the result precision, when we know that 
those bits are zero in either of the inputs, won't impact any more significant 
bits in the result.

Maybe your solution is easier and good enough for a first implementation. And 
maybe `opt` is smart enough to optimize this anyway. Or maybe codegen could be 
worse due to not using legal types. Or maybe codegen could be improved due to 
using sadd.sat in more cases?
Lots of "maybes", so I don't think you need to do anything about this right 
now. It is just an idea from my side.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3252
+  llvm::Function *intrinsic = CGF.CGM.getIntrinsic(IID, CommonTy);
+  Result = Builder.CreateCall(intrinsic, {LHS, RHS});
+}

It should be possible to do it like this (if you add some line wrapping):
```
Builder.CreateBinaryIntrinsic(ResultSign ? llvm::Intrinsic::sadd_sat : 
 llvm::Intrinsic::uadd_sat, LHS, RHS);

```


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] D53707: [Fixed Point Arithmetic] Refactor fixed point casts

[Bjorn Pettersson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC345398: [Fixed Point Arithmetic] Refactor fixed point casts 
(authored by bjope, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D53707?vs=171113&id=171311#toc

Repository:
  rC Clang

https://reviews.llvm.org/D53707

Files:
  lib/CodeGen/CGExprScalar.cpp
  test/Frontend/fixed_point_conversions.c
  test/Frontend/fixed_point_to_bool.c

Index: lib/CodeGen/CGExprScalar.cpp
===
--- lib/CodeGen/CGExprScalar.cpp
+++ lib/CodeGen/CGExprScalar.cpp
@@ -1029,7 +1029,7 @@
   // We do not need to check the padding bit on unsigned types if unsigned
   // padding is enabled because overflow into this bit is undefined
   // behavior.
-  return Builder.CreateIsNotNull(Src);
+  return Builder.CreateIsNotNull(Src, "tobool");
 }
 
 llvm_unreachable(
@@ -1247,79 +1247,62 @@
   unsigned DstWidth = DstFPSema.getWidth();
   unsigned SrcScale = SrcFPSema.getScale();
   unsigned DstScale = DstFPSema.getScale();
-  bool IsSigned = SrcFPSema.isSigned();
+  bool SrcIsSigned = SrcFPSema.isSigned();
+  bool DstIsSigned = DstFPSema.isSigned();
+
+  llvm::Type *DstIntTy = Builder.getIntNTy(DstWidth);
 
   Value *Result = Src;
   unsigned ResultWidth = SrcWidth;
 
   if (!DstFPSema.isSaturated()) {
-// Downscale
-if (DstScale < SrcScale) {
-  if (IsSigned)
-Result = Builder.CreateAShr(Result, SrcScale - DstScale);
-  else
-Result = Builder.CreateLShr(Result, SrcScale - DstScale);
-}
+// Downscale.
+if (DstScale < SrcScale)
+  Result = SrcIsSigned ?
+  Builder.CreateAShr(Result, SrcScale - DstScale, "downscale") :
+  Builder.CreateLShr(Result, SrcScale - DstScale, "downscale");
 
-// Resize
-llvm::Type *DstIntTy = Builder.getIntNTy(DstWidth);
-if (IsSigned)
-  Result = Builder.CreateSExtOrTrunc(Result, DstIntTy);
-else
-  Result = Builder.CreateZExtOrTrunc(Result, DstIntTy);
+// Resize.
+Result = Builder.CreateIntCast(Result, DstIntTy, SrcIsSigned, "resize");
 
-// Upscale
+// Upscale.
 if (DstScale > SrcScale)
-  Result = Builder.CreateShl(Result, DstScale - SrcScale);
+  Result = Builder.CreateShl(Result, DstScale - SrcScale, "upscale");
   } else {
+// Adjust the number of fractional bits.
 if (DstScale > SrcScale) {
-  // Need to extend first before scaling up
   ResultWidth = SrcWidth + DstScale - SrcScale;
   llvm::Type *UpscaledTy = Builder.getIntNTy(ResultWidth);
-
-  if (IsSigned)
-Result = Builder.CreateSExt(Result, UpscaledTy);
-  else
-Result = Builder.CreateZExt(Result, UpscaledTy);
-
-  Result = Builder.CreateShl(Result, DstScale - SrcScale);
+  Result = Builder.CreateIntCast(Result, UpscaledTy, SrcIsSigned, "resize");
+  Result = Builder.CreateShl(Result, DstScale - SrcScale, "upscale");
 } else if (DstScale < SrcScale) {
-  if (IsSigned)
-Result = Builder.CreateAShr(Result, SrcScale - DstScale);
-  else
-Result = Builder.CreateLShr(Result, SrcScale - DstScale);
+  Result = SrcIsSigned ?
+  Builder.CreateAShr(Result, SrcScale - DstScale, "downscale") :
+  Builder.CreateLShr(Result, SrcScale - DstScale, "downscale");
 }
 
-if (DstFPSema.getIntegralBits() < SrcFPSema.getIntegralBits()) {
-  auto Max = ConstantInt::get(
+// Handle saturation.
+bool LessIntBits = DstFPSema.getIntegralBits() < SrcFPSema.getIntegralBits();
+if (LessIntBits) {
+  Value *Max = ConstantInt::get(
   CGF.getLLVMContext(),
   APFixedPoint::getMax(DstFPSema).getValue().extOrTrunc(ResultWidth));
-  Value *TooHigh = IsSigned ? Builder.CreateICmpSGT(Result, Max)
-: Builder.CreateICmpUGT(Result, Max);
-  Result = Builder.CreateSelect(TooHigh, Max, Result);
-
-  if (IsSigned) {
-// Cannot overflow min to dest type is src is unsigned since all fixed
-// point types can cover the unsigned min of 0.
-auto Min = ConstantInt::get(
-CGF.getLLVMContext(),
-APFixedPoint::getMin(DstFPSema).getValue().extOrTrunc(ResultWidth));
-Value *TooLow = Builder.CreateICmpSLT(Result, Min);
-Result = Builder.CreateSelect(TooLow, Min, Result);
-  }
-} else if (IsSigned && !DstFPSema.isSigned()) {
-  llvm::Type *ResultTy = Builder.getIntNTy(ResultWidth);
-  Value *Zero = ConstantInt::getNullValue(ResultTy);
-  Value *LTZero = Builder.CreateICmpSLT(Result, Zero);
-  Result = Builder.CreateSelect(LTZero, Zero, Result);
+  Value *TooHigh = SrcIsSigned ? Builder.CreateICmpSGT(Result, Max)
+   : Builder.CreateICmpUGT(Result, Max);
+  Result = Builder.CreateSelect(TooHigh, Max, Result, "satmax");
+}
+// Cannot overflow min to dest type if src is unsign

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

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang/test/Frontend/fixed_point_comparisons.c:2
+// RUN: %clang_cc1 -ffixed-point -triple x86_64-unknown-linux-gnu -S 
-emit-llvm %s -o - | FileCheck %s --check-prefixes=CHECK,SIGNED
+// RUN: %clang_cc1 -ffixed-point -triple x86_64-unknown-linux-gnu 
-fpadding-on-unsigned-fixed-point -S -emit-llvm %s -o - | FileCheck %s 
--check-prefixes=CHECK,UNSIGNED
+

I think it is easier to understand the checks if you for example use 
UNPADDED/PADDED (or STUDDED/PADDED or something like that) instead of 
SIGNED/UNSIGNED.


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] D56900: [Fixed Point Arithmetic] Fixed Point and Integer Conversions

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1223
   if (SrcType->isFixedPointType()) {
-if (DstType->isFixedPointType()) {
-  return EmitFixedPointConversion(Src, SrcType, DstType, Loc);
-} else if (DstType->isBooleanType()) {
+if (DstType->isBooleanType()) {
+  // It is important that we check this before checking if the dest type is

nit: May skip braces on this if



Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1230
   return Builder.CreateIsNotNull(Src, "tobool");
+} else if (DstType->isFixedPointType() || DstType->isIntegerType()) {
+  return EmitFixedPointConversion(Src, SrcType, DstType, Loc);

nit: may skip braces on this if.
nit: no need to use "else".



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] D57883: [clang-tidy] refactor ExceptionAnalyzer further to give ternary answer

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang-tidy/utils/ExceptionAnalyzer.h:112
+/// throw, if it's unknown or if it won't throw.
+enum State Behaviour : 2;
+

JonasToth wrote:
> bjope wrote:
> > (post-commit comments)
> > 
> > I've seen that @dyung did some VS2015 fixes related to this in 
> > https://reviews.llvm.org/rCTE354545.
> > 
> > But I think there also is some history of problems with typed enums used in 
> > bitfields with GCC (I'm not sure about the details, but it might be 
> > discussed here https://reviews.llvm.org/D24289, and there have been 
> > workarounds applied before, for example here 
> > https://reviews.llvm.org/rCTE319608).
> > 
> > I do not know if we actually have a goal to workaround such problems (no 
> > warnings when using GCC), nor do I know if GCC produce correct code despite 
> > all the warnings we see with GCC (7.4.0) after this patch, such as
> > 
> > ```
> > ../tools/clang/tools/extra/clang-tidy/bugprone/../utils/ExceptionAnalyzer.h:112:23:
> >  warning: 'clang::tidy::utils::ExceptionAnalyzer::ExceptionInfo::Behaviour' 
> > is too small to hold all values of 'enum class 
> > clang::tidy::utils::ExceptionAnalyzer::State'
> >   State Behaviour : 2;
> > ^
> > ../tools/clang/tools/extra/clang-tidy/bugprone/../utils/ExceptionAnalyzer.h:112:23:
> >  warning: 'clang::tidy::utils::ExceptionAnalyzer::ExceptionInfo::Behaviour' 
> > is too small to hold all values of 'enum class 
> > clang::tidy::utils::ExceptionAnalyzer::State'
> >  State Behaviour : 2;
> >^
> > In file included from 
> > ../tools/clang/tools/extra/clang-tidy/utils/ExceptionAnalyzer.cpp:9:0:
> > ../tools/clang/tools/extra/clang-tidy/utils/ExceptionAnalyzer.h:112:23: 
> > warning: 'clang::tidy::utils::ExceptionAnalyzer::ExceptionInfo::Behaviour' 
> > is too small to hold all values of 'enum class 
> > clang::tidy::utils::ExceptionAnalyzer::State'
> >  State Behaviour : 2;
> >^
> > ../tools/clang/tools/extra/clang-tidy/utils/ExceptionAnalyzer.cpp: In 
> > member function 'clang::tidy::utils::ExceptionAnalyzer::ExceptionInfo& 
> > clang::tidy::utils::ExceptionAnalyzer::ExceptionInfo::merge(const 
> > clang::tidy::utils::ExceptionAnalyzer::ExceptionInfo&)':
> > ../tools/clang/tools/extra/clang-tidy/utils/ExceptionAnalyzer.cpp:40:28: 
> > warning: comparison is always false due to limited range of data type 
> > [-Wtype-limits]
> >else if (Other.Behaviour == State::Unknown && Behaviour == 
> > State::NotThrowing)
> > ^~~~
> > ../tools/clang/tools/extra/clang-tidy/utils/ExceptionAnalyzer.cpp:41:24: 
> > warning: overflow in implicit constant conversion [-Woverflow]
> >  Behaviour = State::Unknown;
> > ^~~
> > ../tools/clang/tools/extra/clang-tidy/utils/ExceptionAnalyzer.cpp: In 
> > member function 'void 
> > clang::tidy::utils::ExceptionAnalyzer::ExceptionInfo::reevaluateBehaviour()':
> > ../tools/clang/tools/extra/clang-tidy/utils/ExceptionAnalyzer.cpp:105:26: 
> > warning: overflow in implicit constant conversion [-Woverflow]
> >Behaviour = State::Unknown;
> >   ^~~
> > ```
> > 
> > To sum up:
> > The warning are a little bit annoying (but maybe something we have to live 
> > with if this is known bugs in gcc).
> > It seems like we have done other workarounds in the past (writing ugly code 
> > to satisfy MSVC and GCC). So should we do that here as well?
> > Might wanna check if GCC produce correct code for this.
> I see. All i wanted to achieve was to make the object <= 64 bytes to fit in a 
> cacheline. IMHO the bitfields can disapear. I dont want to sacrifice the 
> `enum class` as I feel that gives more value then the (unmeasured) potential 
> performance gain from shrinking the object sizes.
> 
> I will commit to remove the bitfields. Details can be figured out later.
( regarding https://reviews.llvm.org/rGc1e8cbd5c3f0ed33ab4fb8df98645ebea0018fe8 
)

Pity if we have to sacrifice performance if these are false warnings from gcc. 
But I don't really have the details about that, so thanks for the fix!



Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57883



___
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

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang/test/Frontend/fixed_point_conversions.c:45
+
+short _Accum sa_const9 = 2u; // DEFAULT-DAG: @sa_const9 = {{.*}}global i16 
256, align 2
+unsigned short _Accum usa_const3 = 2;

Perhaps we should verify that we do not mess up signed values when using the 
-fpadding-on-unsigned-fixed-point option (having CHECK, DEFAULT and SAME as 
check prefixes)?

(nit: I also see that we now use different names for this check prefixes in 
different test files, which could be confusing. Why is it called SAME here?) 






Comment at: clang/test/Frontend/fixed_point_conversions.c:66
 
+_Sat short _Accum sat_sa_const3 = 256;  // DEFAULT-DAG: @sat_sa_const3 = 
{{.*}}global i16 32767, align 2
+_Sat short _Accum sat_sa_const4 = -257; // DEFAULT-DAG: @sat_sa_const4 = 
{{.*}}global i16 -32768, align 2

A little bit confusing to be with this mix of having checks left-aligned (on 
separate lines) and some checks appended like this.
I can see that it already is like that before, so maybe it should be cleaned up 
in a separate commit (before this commit).

(I'm not so familiar with clang tests, so maybe it is common to have the 
FileCheck checks at the end of the line. But I've never seen that in llvm.)


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] D53701: [Analyzer] Instead of recording comparisons in interator checkers do an eager state split

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:825
+  // not then create a new symbol for the offset.
+  SymbolRef Sym;
+  if (!LPos || !RPos) {

This is an uninitialized version of `Sym` that will be used on line 835 and 
line 839.
The `Sym` variable assigned on line 828 is local to the scope starting at line 
826.

Not really sure, but was perhaps the idea to use the `Sym` value from line 828 
on line 835 and 839.
Then I guess the you can rewrite this as:


```
  // At least one of the iterators have recorded positions. If one of them has
  // not then create a new symbol for the offset.
  if (!LPos || !RPos) {
auto &SymMgr = C.getSymbolManager();
auto Sym = SymMgr.conjureSymbol(CE, C.getLocationContext(),
C.getASTContext().LongTy, C.blockCount());
State = assumeNoOverflow(State, Sym, 4);

if (!LPos) {
  State = setIteratorPosition(State, LVal,
  IteratorPosition::getPosition(Cont, Sym));
  LPos = getIteratorPosition(State, LVal);
} else if (!RPos) {
  State = setIteratorPosition(State, RVal,
  IteratorPosition::getPosition(Cont, Sym));
  RPos = getIteratorPosition(State, RVal);
}
  }
```

As it is right now I get complaint about using an uninitialized value for `Sym` 
(so this patch still doesn't build with -Werror after the earlier fixup).


Repository:
  rC Clang

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

https://reviews.llvm.org/D53701



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

[PATCH] D57883: [clang-tidy] refactor ExceptionAnalyzer further to give ternary answer

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added subscribers: dyung, bjope.
bjope added inline comments.
Herald added a subscriber: Charusso.



Comment at: clang-tidy/utils/ExceptionAnalyzer.h:112
+/// throw, if it's unknown or if it won't throw.
+enum State Behaviour : 2;
+

(post-commit comments)

I've seen that @dyung did some VS2015 fixes related to this in 
https://reviews.llvm.org/rCTE354545.

But I think there also is some history of problems with typed enums used in 
bitfields with GCC (I'm not sure about the details, but it might be discussed 
here https://reviews.llvm.org/D24289, and there have been workarounds applied 
before, for example here https://reviews.llvm.org/rCTE319608).

I do not know if we actually have a goal to workaround such problems (no 
warnings when using GCC), nor do I know if GCC produce correct code despite all 
the warnings we see with GCC (7.4.0) after this patch, such as

```
../tools/clang/tools/extra/clang-tidy/bugprone/../utils/ExceptionAnalyzer.h:112:23:
 warning: 'clang::tidy::utils::ExceptionAnalyzer::ExceptionInfo::Behaviour' is 
too small to hold all values of 'enum class 
clang::tidy::utils::ExceptionAnalyzer::State'
  State Behaviour : 2;
^
../tools/clang/tools/extra/clang-tidy/bugprone/../utils/ExceptionAnalyzer.h:112:23:
 warning: 'clang::tidy::utils::ExceptionAnalyzer::ExceptionInfo::Behaviour' is 
too small to hold all values of 'enum class 
clang::tidy::utils::ExceptionAnalyzer::State'
 State Behaviour : 2;
   ^
In file included from 
../tools/clang/tools/extra/clang-tidy/utils/ExceptionAnalyzer.cpp:9:0:
../tools/clang/tools/extra/clang-tidy/utils/ExceptionAnalyzer.h:112:23: 
warning: 'clang::tidy::utils::ExceptionAnalyzer::ExceptionInfo::Behaviour' is 
too small to hold all values of 'enum class 
clang::tidy::utils::ExceptionAnalyzer::State'
 State Behaviour : 2;
   ^
../tools/clang/tools/extra/clang-tidy/utils/ExceptionAnalyzer.cpp: In member 
function 'clang::tidy::utils::ExceptionAnalyzer::ExceptionInfo& 
clang::tidy::utils::ExceptionAnalyzer::ExceptionInfo::merge(const 
clang::tidy::utils::ExceptionAnalyzer::ExceptionInfo&)':
../tools/clang/tools/extra/clang-tidy/utils/ExceptionAnalyzer.cpp:40:28: 
warning: comparison is always false due to limited range of data type 
[-Wtype-limits]
   else if (Other.Behaviour == State::Unknown && Behaviour == 
State::NotThrowing)
^~~~
../tools/clang/tools/extra/clang-tidy/utils/ExceptionAnalyzer.cpp:41:24: 
warning: overflow in implicit constant conversion [-Woverflow]
 Behaviour = State::Unknown;
^~~
../tools/clang/tools/extra/clang-tidy/utils/ExceptionAnalyzer.cpp: In member 
function 'void 
clang::tidy::utils::ExceptionAnalyzer::ExceptionInfo::reevaluateBehaviour()':
../tools/clang/tools/extra/clang-tidy/utils/ExceptionAnalyzer.cpp:105:26: 
warning: overflow in implicit constant conversion [-Woverflow]
   Behaviour = State::Unknown;
  ^~~
```

To sum up:
The warning are a little bit annoying (but maybe something we have to live with 
if this is known bugs in gcc).
It seems like we have done other workarounds in the past (writing ugly code to 
satisfy MSVC and GCC). So should we do that here as well?
Might wanna check if GCC produce correct code for this.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57883



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

[PATCH] D57883: [clang-tidy] refactor ExceptionAnalyzer further to give ternary answer

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a subscriber: sammccall.
bjope added a comment.

Maybe @sammccall remembers why it was decided to rewrite the enum into 
constexpr:s in https://reviews.llvm.org/rCTE319608 ? Do we need a similar 
solution here?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D57883



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

[PATCH] D37254: [Sema] Disallow assigning record lvalues with nested const-qualified fields.

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5888
 
 def note_typecheck_assign_const : Note<
   "%select{"

Shouldn't there be one more entry here as well?
(see comment about updating both err_typecheck_assign_const and 
note_typecheck_assign_const when adding things to the enum at line 10181).

If I understand it correctly you never print notes for the new 
NestedConstMember, so there isn't really a need for a fifth entry in the select 
today.
But if someone adds new enum values later (putting them after NestedContMember) 
then it might be nice if the size of this select matches with the old size of 
the enum.




Comment at: lib/Sema/SemaExpr.cpp:10329
+  S.Diag(Field->getLocation(), diag::note_typecheck_assign_const)
+  << ConstMember << false /*non-static*/ << Field
+  << Field->getType() << Field->getSourceRange();

If adding a new entry to note_typecheck_assign_const (identical to the 
ConstMember entry) then you can use NestedConstMember here. It would make it 
possible to change the string for NestedConstMember notes without also changing 
all ConstMember notes.

Now it kind of looks like an error that the error diagnostic is for a 
NestedConstMember, but the note is about a ConstMember. That might confuse 
someone, since it looks like a typo unless you know what is going on here.


https://reviews.llvm.org/D37254



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

[PATCH] D37254: [Sema] Disallow assigning record lvalues with nested const-qualified fields.

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

This patch looks good to me now.
But I'm currently working in the same out-of-tree project as the author, so it 
would be nice if someone else could have a look as well and accept this.


https://reviews.llvm.org/D37254



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

[PATCH] D37254: [Sema] Disallow assigning record lvalues with nested const-qualified fields.

[Bjorn Pettersson via Phabricator via cfe-commits]

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

Nobody else seems to bother. At least no objections, so I think we can land 
this now.


https://reviews.llvm.org/D37254



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

[PATCH] D37254: [Sema] Disallow assigning record lvalues with nested const-qualified fields.

[Bjorn Pettersson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL313628: [Sema] Disallow assigning record lvalues with nested 
const-qualified fields. (authored by bjope).

Changed prior to commit:
  https://reviews.llvm.org/D37254?vs=114340&id=115831#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D37254

Files:
  cfe/trunk/include/clang/AST/Expr.h
  cfe/trunk/include/clang/AST/Type.h
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/AST/ExprClassification.cpp
  cfe/trunk/lib/AST/Type.cpp
  cfe/trunk/lib/Sema/SemaExpr.cpp
  cfe/trunk/test/Sema/assign.c

Index: cfe/trunk/include/clang/AST/Type.h
===
--- cfe/trunk/include/clang/AST/Type.h
+++ cfe/trunk/include/clang/AST/Type.h
@@ -3791,10 +3791,9 @@
 return reinterpret_cast(TagType::getDecl());
   }
 
-  // FIXME: This predicate is a helper to QualType/Type. It needs to
-  // recursively check all fields for const-ness. If any field is declared
-  // const, it needs to return false.
-  bool hasConstFields() const { return false; }
+  /// Recursively check all fields in the record for const-ness. If any field
+  /// is declared const, return true. Otherwise, return false.
+  bool hasConstFields() const;
 
   bool isSugared() const { return false; }
   QualType desugar() const { return QualType(this, 0); }
Index: cfe/trunk/include/clang/AST/Expr.h
===
--- cfe/trunk/include/clang/AST/Expr.h
+++ cfe/trunk/include/clang/AST/Expr.h
@@ -275,6 +275,7 @@
 MLV_LValueCast,   // Specialized form of MLV_InvalidExpression.
 MLV_IncompleteType,
 MLV_ConstQualified,
+MLV_ConstQualifiedField,
 MLV_ConstAddrSpace,
 MLV_ArrayType,
 MLV_NoSetterProperty,
@@ -324,6 +325,7 @@
   CM_LValueCast, // Same as CM_RValue, but indicates GCC cast-as-lvalue ext
   CM_NoSetterProperty,// Implicit assignment to ObjC property without setter
   CM_ConstQualified,
+  CM_ConstQualifiedField,
   CM_ConstAddrSpace,
   CM_ArrayType,
   CM_IncompleteType
Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -5888,14 +5888,17 @@
   "cannot assign to %select{non-|}1static data member %2 "
   "with const-qualified type %3|"
   "cannot assign to non-static data member within const member function %1|"
+  "cannot assign to %select{variable %2|non-static data member %2|lvalue}1 "
+  "with %select{|nested }3const-qualified data member %4|"
   "read-only variable is not assignable}0">;
 
 def note_typecheck_assign_const : Note<
   "%select{"
   "function %1 which returns const-qualified type %2 declared here|"
   "variable %1 declared const here|"
   "%select{non-|}1static data member %2 declared const here|"
-  "member function %q1 is declared const here}0">;
+  "member function %q1 is declared const here|"
+  "%select{|nested }1data member %2 declared const here}0">;
 
 def warn_mixed_sign_comparison : Warning<
   "comparison of integers of different signs: %0 and %1">,
Index: cfe/trunk/test/Sema/assign.c
===
--- cfe/trunk/test/Sema/assign.c
+++ cfe/trunk/test/Sema/assign.c
@@ -16,3 +16,46 @@
   b[4] = 1; // expected-error {{read-only variable is not assignable}}
   b2[4] = 1; // expected-error {{read-only variable is not assignable}}
 }
+
+typedef struct I {
+  const int a; // expected-note 4{{nested data member 'a' declared const here}} \
+  expected-note 6{{data member 'a' declared const here}}
+} I;
+typedef struct J {
+  struct I i;
+} J;
+typedef struct K {
+  struct J *j;
+} K;
+
+void testI(struct I i1, struct I i2) {
+  i1 = i2; // expected-error {{cannot assign to variable 'i1' with const-qualified data member 'a'}}
+}
+void testJ1(struct J j1, struct J j2) {
+  j1 = j2; // expected-error {{cannot assign to variable 'j1' with nested const-qualified data member 'a'}}
+}
+void testJ2(struct J j, struct I i) {
+  j.i = i; // expected-error {{cannot assign to non-static data member 'i' with const-qualified data member 'a'}}
+}
+void testK1(struct K k, struct J j) {
+  *(k.j) = j; // expected-error {{cannot assign to lvalue with nested const-qualified data member 'a'}}
+}
+void testK2(struct K k, struct I i) {
+  k.j->i = i; // expected-error {{cannot assign to non-static data member 'i' with const-qualified data member 'a'}}
+}
+
+void testI_(I i1, I i2) {
+  i1 = i2; // expected-error {{cannot assign to variable 'i1' with const-qualified data member 'a'}}
+}
+void testJ1_(J j1, J j2) {
+  j1 = j2; // expected-error {{cannot assign to variable 'j1' with nested const-qualified data member 'a'}}
+}
+void testJ2_(J j, I i) {
+  j.i = i; // expected-error {{cannot assign to non-static data m

[PATCH] D65233: driver: Don't warn about assembler flags being unused when not assembling; different approach

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

I made another fixup (similar to https://reviews.llvm.org/rL367176) here 
https://reviews.llvm.org/rL367182.

Can someone please take a look (a post-commit review) of 
https://reviews.llvm.org/rL367182 to verify that I did not misunderstand the 
intention with the test somehow?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65233



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

[PATCH] D65233: driver: Don't warn about assembler flags being unused when not assembling; different approach

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

In D65233#1603783 , @thakis wrote:

> In D65233#1603741 , @bjope wrote:
>
> > I made another fixup (similar to https://reviews.llvm.org/rL367176) here 
> > https://reviews.llvm.org/rL367182.
> >
> > Can someone please take a look (a post-commit review) of 
> > https://reviews.llvm.org/rL367182 to verify that I did not misunderstand 
> > the intention with the test somehow?
>
>
> Looks good. Thanks much for the fix!


Nice to know that I did not mess up the test in my attempt to silence the 
buildbots. So thanks for taking a look.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65233



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

[PATCH] D63518: BitStream reader: propagate errors

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp:619
+// FIXME check that the enum is in range.
+auto Code = static_cast(MaybeCode.get());
+

This has caused problem for our sanitizer tests the last couple of days:

```
FAIL: Extra Tools Unit Tests :: 
clang-doc/./ClangDocTests/BitcodeTest.emitMethodInfoBitcode (20077 of 48746)
 TEST 'Extra Tools Unit Tests :: 
clang-doc/./ClangDocTests/BitcodeTest.emitMethodInfoBitcode' FAILED 

Note: Google Test filter = BitcodeTest.emitMethodInfoBitcode
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from BitcodeTest
[ RUN  ] BitcodeTest.emitMethodInfoBitcode
/local/repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master-sanitize/clang-tools-extra/clang-doc/BitcodeReader.cpp:621:13:
 runtime error: load of value 9, which is not a valid value for type 
'llvm::bitc::FixedAbbrevIDs'

```

This was seen when building trunk with clang 6.0.0 and 
LVM_USE_SANITIZER=Undefined

```
cmake -G Ninja '-DLLVM_ENABLE_PROJECTS='\''clang;clang-tools-extra'\''' 
-DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON 
'-DCMAKE_CXX_FLAGS='\''-stdlib=libc++'\''' -DCMAKE_C_COMPILER=clang 
-DCMAKE_CXX_COMPILER=clang++ -DLLVM_USE_LINKER=gold 
-DLLVM_USE_SANITIZER=Undefined ../.
-- The C compiler identification is Clang 6.0.0
-- The CXX compiler identification is Clang 6.0.0
```

Afaict we can't cast the read value to FixedAbbrevIDs as we do not know yet if 
it matches one of the values defined in the enum, or if we will take the 
default case.


A similar switch exist at 
cfe/trunk/lib/Frontend/SerializedDiagnosticReader.cpp:127, but it is using a 
slightly different pattern:

```
unsigned Code;
Code = Res.get();
switch ((llvm::bitc::FixedAbbrevIDs)Code) 
```
I haven't seen any failures for SerializedDiagnosticReader. So either we lack 
test coverage for that function, or the sanitizer only introduce the check when 
using the static_cast (and declaring Code as an enum) as done here.



Repository:
  rL LLVM

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

https://reviews.llvm.org/D63518



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

[PATCH] D64262: Bitstream reader: Fix undefined behavior seen after rL364464

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope created this revision.
bjope added a reviewer: jfb.
Herald added subscribers: kadircet, dexonsmith, ilya-biryukov.
Herald added a project: clang.

After rL364464  the following tests started 
to fail when
running the clang-doc tests with an ubsan instrumented
build of clang-doc:

  Clang Tools :: clang-doc/single-file-public.cpp
  Extra Tools Unit Tests :: 
clang-doc/./ClangDocTests/BitcodeTest.emitEnumInfoBitcode
  Extra Tools Unit Tests :: 
clang-doc/./ClangDocTests/BitcodeTest.emitMethodInfoBitcode
  Extra Tools Unit Tests :: 
clang-doc/./ClangDocTests/BitcodeTest.emitRecordInfoBitcode
  Extra Tools Unit Tests :: 
clang-doc/./ClangDocTests/SerializeTest.emitInfoWithCommentBitcode

We need to check that the read value is in range for being
casted to the llvm::bitc::FixedAbbrevIDs enum, before the
cast in ClangDocBitcodeReader::skipUntilRecordOrBlock.

SerializedDiagnosticReader::skipUntilRecordOrBlock was updated
in the same way.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D64262

Files:
  clang-tools-extra/clang-doc/BitcodeReader.cpp
  clang/lib/Frontend/SerializedDiagnosticReader.cpp


Index: clang/lib/Frontend/SerializedDiagnosticReader.cpp
===
--- clang/lib/Frontend/SerializedDiagnosticReader.cpp
+++ clang/lib/Frontend/SerializedDiagnosticReader.cpp
@@ -124,7 +124,12 @@
 else
   return llvm::errorToErrorCode(Res.takeError());
 
-switch ((llvm::bitc::FixedAbbrevIDs)Code) {
+if (Code >= static_cast(llvm::bitc::FIRST_APPLICATION_ABBREV)) {
+  // We found a record.
+  BlockOrRecordID = Code;
+  return Cursor::Record;
+}
+switch (static_cast(Code)) {
 case llvm::bitc::ENTER_SUBBLOCK:
   if (Expected Res = Stream.ReadSubBlockID())
 BlockOrRecordID = Res.get();
@@ -145,10 +150,8 @@
 case llvm::bitc::UNABBREV_RECORD:
   return SDError::UnsupportedConstruct;
 
-default:
-  // We found a record.
-  BlockOrRecordID = Code;
-  return Cursor::Record;
+case llvm::bitc::FIRST_APPLICATION_ABBREV:
+  llvm_unreachable("Unexpected abbrev id.");
 }
   }
 
Index: clang-tools-extra/clang-doc/BitcodeReader.cpp
===
--- clang-tools-extra/clang-doc/BitcodeReader.cpp
+++ clang-tools-extra/clang-doc/BitcodeReader.cpp
@@ -615,10 +615,12 @@
   return Cursor::BadBlock;
 }
 
-// FIXME check that the enum is in range.
-auto Code = static_cast(MaybeCode.get());
-
-switch (Code) {
+unsigned Code = MaybeCode.get();
+if (Code >= static_cast(llvm::bitc::FIRST_APPLICATION_ABBREV)) {
+  BlockOrRecordID = Code;
+  return Cursor::Record;
+}
+switch (static_cast(Code)) {
 case llvm::bitc::ENTER_SUBBLOCK:
   if (Expected MaybeID = Stream.ReadSubBlockID())
 BlockOrRecordID = MaybeID.get();
@@ -639,9 +641,8 @@
   continue;
 case llvm::bitc::UNABBREV_RECORD:
   return Cursor::BadBlock;
-default:
-  BlockOrRecordID = Code;
-  return Cursor::Record;
+case llvm::bitc::FIRST_APPLICATION_ABBREV:
+  llvm_unreachable("Unexpected abbrev id.");
 }
   }
   llvm_unreachable("Premature stream end.");


Index: clang/lib/Frontend/SerializedDiagnosticReader.cpp
===
--- clang/lib/Frontend/SerializedDiagnosticReader.cpp
+++ clang/lib/Frontend/SerializedDiagnosticReader.cpp
@@ -124,7 +124,12 @@
 else
   return llvm::errorToErrorCode(Res.takeError());
 
-switch ((llvm::bitc::FixedAbbrevIDs)Code) {
+if (Code >= static_cast(llvm::bitc::FIRST_APPLICATION_ABBREV)) {
+  // We found a record.
+  BlockOrRecordID = Code;
+  return Cursor::Record;
+}
+switch (static_cast(Code)) {
 case llvm::bitc::ENTER_SUBBLOCK:
   if (Expected Res = Stream.ReadSubBlockID())
 BlockOrRecordID = Res.get();
@@ -145,10 +150,8 @@
 case llvm::bitc::UNABBREV_RECORD:
   return SDError::UnsupportedConstruct;
 
-default:
-  // We found a record.
-  BlockOrRecordID = Code;
-  return Cursor::Record;
+case llvm::bitc::FIRST_APPLICATION_ABBREV:
+  llvm_unreachable("Unexpected abbrev id.");
 }
   }
 
Index: clang-tools-extra/clang-doc/BitcodeReader.cpp
===
--- clang-tools-extra/clang-doc/BitcodeReader.cpp
+++ clang-tools-extra/clang-doc/BitcodeReader.cpp
@@ -615,10 +615,12 @@
   return Cursor::BadBlock;
 }
 
-// FIXME check that the enum is in range.
-auto Code = static_cast(MaybeCode.get());
-
-switch (Code) {
+unsigned Code = MaybeCode.get();
+if (Code >= static_cast(llvm::bitc::FIRST_APPLICATION_ABBREV)) {
+  BlockOrRecordID = Code;
+  return Cursor::Record;
+}
+switch (static_cast(Code)) {
 case llvm::bitc::ENTER_SUBBLOCK:
   if (Expected MaybeID = Stream.ReadSubBlockID

[PATCH] D63518: BitStream reader: propagate errors

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp:619
+// FIXME check that the enum is in range.
+auto Code = static_cast(MaybeCode.get());
+

jfb wrote:
> jfb wrote:
> > bjope wrote:
> > > This has caused problem for our sanitizer tests the last couple of days:
> > > 
> > > ```
> > > FAIL: Extra Tools Unit Tests :: 
> > > clang-doc/./ClangDocTests/BitcodeTest.emitMethodInfoBitcode (20077 of 
> > > 48746)
> > >  TEST 'Extra Tools Unit Tests :: 
> > > clang-doc/./ClangDocTests/BitcodeTest.emitMethodInfoBitcode' FAILED 
> > > 
> > > Note: Google Test filter = BitcodeTest.emitMethodInfoBitcode
> > > [==] Running 1 test from 1 test case.
> > > [--] Global test environment set-up.
> > > [--] 1 test from BitcodeTest
> > > [ RUN  ] BitcodeTest.emitMethodInfoBitcode
> > > /local/repo/bbiswjenk/fem023-eiffel003/workspace/llvm/llvm-master-sanitize/clang-tools-extra/clang-doc/BitcodeReader.cpp:621:13:
> > >  runtime error: load of value 9, which is not a valid value for type 
> > > 'llvm::bitc::FixedAbbrevIDs'
> > > 
> > > ```
> > > 
> > > This was seen when building trunk with clang 6.0.0 and 
> > > LVM_USE_SANITIZER=Undefined
> > > 
> > > ```
> > > cmake -G Ninja '-DLLVM_ENABLE_PROJECTS='\''clang;clang-tools-extra'\''' 
> > > -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON 
> > > '-DCMAKE_CXX_FLAGS='\''-stdlib=libc++'\''' -DCMAKE_C_COMPILER=clang 
> > > -DCMAKE_CXX_COMPILER=clang++ -DLLVM_USE_LINKER=gold 
> > > -DLLVM_USE_SANITIZER=Undefined ../.
> > > -- The C compiler identification is Clang 6.0.0
> > > -- The CXX compiler identification is Clang 6.0.0
> > > ```
> > > 
> > > Afaict we can't cast the read value to FixedAbbrevIDs as we do not know 
> > > yet if it matches one of the values defined in the enum, or if we will 
> > > take the default case.
> > > 
> > > 
> > > A similar switch exist at 
> > > cfe/trunk/lib/Frontend/SerializedDiagnosticReader.cpp:127, but it is 
> > > using a slightly different pattern:
> > > 
> > > ```
> > > unsigned Code;
> > > Code = Res.get();
> > > switch ((llvm::bitc::FixedAbbrevIDs)Code) 
> > > ```
> > > I haven't seen any failures for SerializedDiagnosticReader. So either we 
> > > lack test coverage for that function, or the sanitizer only introduce the 
> > > check when using the static_cast (and declaring Code as an enum) as done 
> > > here.
> > > 
> > That sounds like a pre-existing bug. We should check that the value is in 
> > range before casting. Can you send patches to fix both code locations, and 
> > add test coverage? This code is indeed poorly tested.
> > 
> > Why do the sanitizers catch `static_cast` but not C-style casts?
> To be clear: relying on the `default` case is still UB because there's a cast 
> to the enum type before it occurs.
I made a patch here (assuming the goal would be to keep the cast to the enum, 
and to let the switch cover all enum values): https://reviews.llvm.org/D64262


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63518



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

[PATCH] D64262: Bitstream reader: Fix undefined behavior seen after rL364464

[Bjorn Pettersson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL365239: Bitstream reader: Fix undefined behavior seen after 
rL364464 (authored by bjope, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64262?vs=208222&id=208226#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64262

Files:
  cfe/trunk/lib/Frontend/SerializedDiagnosticReader.cpp
  clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp


Index: clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp
===
--- clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp
+++ clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp
@@ -615,10 +615,12 @@
   return Cursor::BadBlock;
 }
 
-// FIXME check that the enum is in range.
-auto Code = static_cast(MaybeCode.get());
-
-switch (Code) {
+unsigned Code = MaybeCode.get();
+if (Code >= static_cast(llvm::bitc::FIRST_APPLICATION_ABBREV)) {
+  BlockOrRecordID = Code;
+  return Cursor::Record;
+}
+switch (static_cast(Code)) {
 case llvm::bitc::ENTER_SUBBLOCK:
   if (Expected MaybeID = Stream.ReadSubBlockID())
 BlockOrRecordID = MaybeID.get();
@@ -639,9 +641,8 @@
   continue;
 case llvm::bitc::UNABBREV_RECORD:
   return Cursor::BadBlock;
-default:
-  BlockOrRecordID = Code;
-  return Cursor::Record;
+case llvm::bitc::FIRST_APPLICATION_ABBREV:
+  llvm_unreachable("Unexpected abbrev id.");
 }
   }
   llvm_unreachable("Premature stream end.");
Index: cfe/trunk/lib/Frontend/SerializedDiagnosticReader.cpp
===
--- cfe/trunk/lib/Frontend/SerializedDiagnosticReader.cpp
+++ cfe/trunk/lib/Frontend/SerializedDiagnosticReader.cpp
@@ -124,7 +124,12 @@
 else
   return llvm::errorToErrorCode(Res.takeError());
 
-switch ((llvm::bitc::FixedAbbrevIDs)Code) {
+if (Code >= static_cast(llvm::bitc::FIRST_APPLICATION_ABBREV)) {
+  // We found a record.
+  BlockOrRecordID = Code;
+  return Cursor::Record;
+}
+switch (static_cast(Code)) {
 case llvm::bitc::ENTER_SUBBLOCK:
   if (Expected Res = Stream.ReadSubBlockID())
 BlockOrRecordID = Res.get();
@@ -145,10 +150,8 @@
 case llvm::bitc::UNABBREV_RECORD:
   return SDError::UnsupportedConstruct;
 
-default:
-  // We found a record.
-  BlockOrRecordID = Code;
-  return Cursor::Record;
+case llvm::bitc::FIRST_APPLICATION_ABBREV:
+  llvm_unreachable("Unexpected abbrev id.");
 }
   }
 


Index: clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp
===
--- clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp
+++ clang-tools-extra/trunk/clang-doc/BitcodeReader.cpp
@@ -615,10 +615,12 @@
   return Cursor::BadBlock;
 }
 
-// FIXME check that the enum is in range.
-auto Code = static_cast(MaybeCode.get());
-
-switch (Code) {
+unsigned Code = MaybeCode.get();
+if (Code >= static_cast(llvm::bitc::FIRST_APPLICATION_ABBREV)) {
+  BlockOrRecordID = Code;
+  return Cursor::Record;
+}
+switch (static_cast(Code)) {
 case llvm::bitc::ENTER_SUBBLOCK:
   if (Expected MaybeID = Stream.ReadSubBlockID())
 BlockOrRecordID = MaybeID.get();
@@ -639,9 +641,8 @@
   continue;
 case llvm::bitc::UNABBREV_RECORD:
   return Cursor::BadBlock;
-default:
-  BlockOrRecordID = Code;
-  return Cursor::Record;
+case llvm::bitc::FIRST_APPLICATION_ABBREV:
+  llvm_unreachable("Unexpected abbrev id.");
 }
   }
   llvm_unreachable("Premature stream end.");
Index: cfe/trunk/lib/Frontend/SerializedDiagnosticReader.cpp
===
--- cfe/trunk/lib/Frontend/SerializedDiagnosticReader.cpp
+++ cfe/trunk/lib/Frontend/SerializedDiagnosticReader.cpp
@@ -124,7 +124,12 @@
 else
   return llvm::errorToErrorCode(Res.takeError());
 
-switch ((llvm::bitc::FixedAbbrevIDs)Code) {
+if (Code >= static_cast(llvm::bitc::FIRST_APPLICATION_ABBREV)) {
+  // We found a record.
+  BlockOrRecordID = Code;
+  return Cursor::Record;
+}
+switch (static_cast(Code)) {
 case llvm::bitc::ENTER_SUBBLOCK:
   if (Expected Res = Stream.ReadSubBlockID())
 BlockOrRecordID = Res.get();
@@ -145,10 +150,8 @@
 case llvm::bitc::UNABBREV_RECORD:
   return SDError::UnsupportedConstruct;
 
-default:
-  // We found a record.
-  BlockOrRecordID = Code;
-  return Cursor::Record;
+case llvm::bitc::FIRST_APPLICATION_ABBREV:
+  llvm_unreachable("Unexpected abbrev id.");
 }
   }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bi

[PATCH] D45217: [ThinLTO] Pass -save-temps to LTO backend for distributed ThinLTO builds

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: cfe/trunk/lib/Driver/ToolChains/Clang.cpp:3270
 
+  if (const Arg *A = Args.getLastArg(options::OPT_save_temps_EQ))
+Args.AddLastArg(CmdArgs, options::OPT_save_temps_EQ);

A is not used, so this does not compile when using -Werror:

```
../tools/clang/lib/Driver/ToolChains/Clang.cpp:3270:18: error: unused variable 
'A' [-Werror,-Wunused-variable]
  if (const Arg *A = Args.getLastArg(options::OPT_save_temps_EQ))

```


Repository:
  rL LLVM

https://reviews.llvm.org/D45217



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

[PATCH] D45619: [Time-report] (1) Use special new Clang flag 'FrontendTimesIsEnabled' instead of 'llvm::TimePassesIsEnabled' inside -ftime-report feature

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

In https://reviews.llvm.org/D45619#1075089, @avt77 wrote:

> It's terrible but my new test was failed again as result of commit of this 
> patch!
>
> ///b/sanitizer-x86_64-linux-bootstrap/build/llvm/tools/clang/test/Frontend/ftime-report-template-decl.cpp:155:11:
>  error: expected string not found in input
>  // CHECK: Code Generation Time
>  //  ^
>
> I don't understand how it's possible. The same problem raised when I 
> committed D43578 . Obviously, there is a 
> situation when this  test work w/o Code Generation and as result this test is 
> fail because code generation time is zerro. Could anyone help me? How should 
> I change the test? The simplest way is to remove this line but I don't like 
> this idea.


Not sure, but is perhaps the order in which timers are printed not 100% 
deterministic?
When I run the test "manually" without pipe to FileCheck I get:

  
===-===
   Miscellaneous Ungrouped Timers
  
===-===
  
 --System Time--   --User+System--   ---Wall Time---  --- Name ---
 0.0040 (100.0%)   0.0040 (100.0%)   0.0021 ( 70.7%)  Code Generation Time
 0. (  0.0%)   0. (  0.0%)   0.0009 ( 29.3%)  LLVM IR Generation 
Time
 0.0040 (100.0%)   0.0040 (100.0%)   0.0030 (100.0%)  Total

So when I run it without the pipe "Code Generation Time" is printed before 
"LLVM IR Generation Time".
However, the CHECK:s in the test case are in opposite order.
So I can't really understand why the test passes when I pipe it to FileCheck.

Anyway, if the order isn't deteministic, then a solution could be to use 
CHECK-DAG instead of CHECK for the checks that may be reordered. For example:

  // CHECK: Miscellaneous Ungrouped Timers
  // CHECK-DAG: Code Generation Time
  // CHECK-DAG: LLVM IR Generation Time
  // CHECK: Total
  // CHECK: Clang front-end time report
  // CHECK: Clang front-end timer
  // CHECK: Total

Unless the timers are supposed to be printed in a certain order, then I guess 
you need to add a sort somewhere in the code.


Repository:
  rL LLVM

https://reviews.llvm.org/D45619



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

[PATCH] D45619: [Time-report] (1) Use special new Clang flag 'FrontendTimesIsEnabled' instead of 'llvm::TimePassesIsEnabled' inside -ftime-report feature

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

I can't see that it has been reverted.
But I guess that the table maybe is sorted based on time spent in each pass? So 
that is why it might be sorted differently on different buildbots (or when 
using pipe etc).

So I think a quick fix is to add -DAG to the checks that can be reorder and 
submit that fix.


Repository:
  rL LLVM

https://reviews.llvm.org/D45619



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

[PATCH] D45619: [Time-report] (1) Use special new Clang flag 'FrontendTimesIsEnabled' instead of 'llvm::TimePassesIsEnabled' inside -ftime-report feature

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

buildbots have been failing all day (and were still failing). So I pushed my 
suggested fix.
I hope that was OK.


Repository:
  rL LLVM

https://reviews.llvm.org/D45619



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

[PATCH] D48721: Patch to fix pragma metadata for do-while loops

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

I tried running

  /clang -cc1 -O3 -funroll-loops -S -emit-llvm pragma-do-while-unroll.cpp -o - 
-mllvm -print-after-all

and I get this

  ...
  !2 = distinct !{!2, !3}
  !3 = !{!"llvm.loop.unroll.count", i32 3}
  !4 = !{!5, !5, i64 0}
  !5 = !{!"int", !6, i64 0}
  !6 = !{!"omnipotent char", !7, i64 0}
  !7 = !{!"Simple C/C++ TBAA"}
  !8 = distinct !{!8, !9}
  !9 = !{!"llvm.loop.unroll.count", i32 5}
  *** IR Dump After Combine redundant instructions ***
  ; Function Attrs: nounwind
  define i32 @test(i32* %a, i32 %n) local_unnamed_addr #0 {
  entry:
br label %do.body, !llvm.loop !2
  
  do.body:  ; preds = %do.body, %entry
%i.0 = phi i32 [ 0, %entry ], [ %inc, %do.body ]
%sum.0 = phi i32 [ 0, %entry ], [ %add5, %do.body ]
%0 = zext i32 %i.0 to i64
%arrayidx = getelementptr inbounds i32, i32* %a, i64 %0
%1 = load i32, i32* %arrayidx, align 4, !tbaa !4
%add = add nsw i32 %1, 1
store i32 %add, i32* %arrayidx, align 4, !tbaa !4
%add5 = add nsw i32 %sum.0, %add
%inc = add nuw nsw i32 %i.0, 1
%cmp = icmp slt i32 %inc, %n
br i1 %cmp, label %do.body, label %do.end, !llvm.loop !2
  
  do.end:   ; preds = %do.body
br label %do.body6, !llvm.loop !8
  
  do.body6: ; preds = %do.body6, %do.end
%i.1 = phi i32 [ 0, %do.end ], [ %inc15, %do.body6 ]
%sum.1 = phi i32 [ %add5, %do.end ], [ %add14, %do.body6 ]
%2 = zext i32 %i.1 to i64
%arrayidx8 = getelementptr inbounds i32, i32* %a, i64 %2
%3 = load i32, i32* %arrayidx8, align 4, !tbaa !4
%add9 = add nsw i32 %3, 1
store i32 %add9, i32* %arrayidx8, align 4, !tbaa !4
%add14 = add nsw i32 %sum.1, %add9
%inc15 = add nuw nsw i32 %i.1, 1
%cmp17 = icmp slt i32 %inc15, %n
br i1 %cmp17, label %do.body6, label %do.end18, !llvm.loop !8
  
  do.end18: ; preds = %do.body6
ret i32 %add14
  }
  *** IR Dump After Simplify the CFG ***
  ; Function Attrs: nounwind
  define i32 @test(i32* %a, i32 %n) local_unnamed_addr #0 {
  entry:
br label %do.body, !llvm.loop !2
  
  do.body:  ; preds = %do.body, %entry
%i.0 = phi i32 [ 0, %entry ], [ %inc, %do.body ]
%sum.0 = phi i32 [ 0, %entry ], [ %add5, %do.body ]
%0 = zext i32 %i.0 to i64
%arrayidx = getelementptr inbounds i32, i32* %a, i64 %0
%1 = load i32, i32* %arrayidx, align 4, !tbaa !4
%add = add nsw i32 %1, 1
store i32 %add, i32* %arrayidx, align 4, !tbaa !4
%add5 = add nsw i32 %sum.0, %add
%inc = add nuw nsw i32 %i.0, 1
%cmp = icmp slt i32 %inc, %n
br i1 %cmp, label %do.body, label %do.body6, !llvm.loop !8
  
  do.body6: ; preds = %do.body, 
%do.body6
%i.1 = phi i32 [ %inc15, %do.body6 ], [ 0, %do.body ]
%sum.1 = phi i32 [ %add14, %do.body6 ], [ %add5, %do.body ]
%2 = zext i32 %i.1 to i64
%arrayidx8 = getelementptr inbounds i32, i32* %a, i64 %2
%3 = load i32, i32* %arrayidx8, align 4, !tbaa !4
%add9 = add nsw i32 %3, 1
store i32 %add9, i32* %arrayidx8, align 4, !tbaa !4
%add14 = add nsw i32 %sum.1, %add9
%inc15 = add nuw nsw i32 %i.1, 1
%cmp17 = icmp slt i32 %inc15, %n
br i1 %cmp17, label %do.body6, label %do.end18, !llvm.loop !8
  
  do.end18: ; preds = %do.body6
ret i32 %add14
  }
  ...

So up until simplifyCFG I think things look pretty good with different 
loop-metadata for the two loops. But when simplifyCFG is tranforming

br i1 %cmp, label %do.body, label %do.end, !llvm.loop !2
  
  do.end:   ; preds = %do.body
br label %do.body6, !llvm.loop !8

into

  br i1 %cmp, label %do.body, label %do.body6, !llvm.loop !8

we get incorrect metadata for one branch target.

Is the fault that the metadata only should be put on the back edge, not the 
branch in the preheader?


Repository:
  rC Clang

https://reviews.llvm.org/D48721



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

[PATCH] D48721: Patch to fix pragma metadata for do-while loops

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

In https://reviews.llvm.org/D48721#1150361, @hfinkel wrote:

> In https://reviews.llvm.org/D48721#1150333, @bjope wrote:
>
> > Is the fault that the metadata only should be put on the back edge, not the 
> > branch in the preheader?
>
>
> Yea. Our past thinking has been that any backedge in the loop is valid. The 
> metadata shouldn't end up other places, although it's benign unless those 
> other places are (or may later become) a backedge for some different loop.


I'm no expert on clang. The patch seems to fix the problem if the goal is to 
only add the loop-metadata on the backedge , but I'll leave it to someone else 
to approve it.

I'm a little bit concerned about opt not detecting this kind of problems though.
Would it be possible for some verifier to detect if we have loop metadata on 
some branch that aren't in the latch block?
And/or should the optimization that "merges" two branches with different loop 
metadata), be smarter about which loop metadata to keep? Or maybe we should be 
defensive and discard loop metadata on the merged branch instruction?


Repository:
  rC Clang

https://reviews.llvm.org/D48721



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

[PATCH] D48721: Patch to fix pragma metadata for do-while loops

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: test/CodeGen/pragma-do-while.cpp:1
+// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s
+int test(int a[], int n) {

I think that we can simplify it to use one loop here (as a regression test that 
we only put the label on one branch).

I also think that the important check is to verify that the llvm.loop is put on 
the branch in the "do.cond" block.

So may I suggest this slightly modified test case:


```
// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s

// We expect to get a loop structure like this:
//do.body:   ; preds = %do.cond, ...
//  ...
//  br label %do.cond
//do.cond:   ; preds = %do.body
//  ...
//  br i1 %cmp, label %do.body, label %do.end
//do.end:; preds = %do.cond
//  ...
//
// Verify that the loop metadata only is put on the backedge.
//
// CHECK-NOT: llvm.loop
// CHECK-LABEL: do.cond:
// CHECK: br {{.*}}, label %do.body, label %do.end, !llvm.loop ![[LMD1:[0-9]+]]
// CHECK-LABEL: do.end:
// CHECK-NOT: llvm.loop
// CHECK: ![[LMD1]] = distinct !{![[LMD1]], ![[LMD2:[0-9]+]]}
// CHECK: ![[LMD2]] = !{!"llvm.loop.unroll.count", i32 4}

int test(int a[], int n) {
  int i = 0;
  int sum = 0;

#pragma unroll 4
  do
  {
a[i] = a[i] + 1;
sum = sum + a[i];
i++;
  } while (i < n);

  return sum;
}
```


https://reviews.llvm.org/D48721



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

[PATCH] D48721: Patch to fix pragma metadata for do-while loops

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

In https://reviews.llvm.org/D48721#1153138, @deepak2427 wrote:

> I have updated the test to not run the optimizer. The test I had added 
> previously for checking if the unroller is respecting the pragma is useful I 
> think. Not sure where that can be added though. 
>  I guess it's independent of this patch anyway. If the patch and test is 
> okay, will update bugzilla as well.


My idea was to forbid the IR that caused problems for the unroller (and other 
optimization passes). Then we do not need to fixup various passes to handle 
"misplaced" llvm.loop annotations correctly. And neither do we need to 
implement test cases to verify that the "misplaced" annotations are handled 
correctly for lots of passes.

I played around a little bit with teaching the IR Verifier in opt to check that 
!llvm.loop only is put in loop latches.
Something like this might do the trick, when added to 
Verifier::visitInstruction in lib/IR/Verifier.cpp:

  #include "llvm/Analysis/LoopInfo.h"
  
  if (I.getMetadata(LLVMContext::MD_loop)) {
// FIXME: Is SwitchInst also allowed?
Assert(isa(I), "llvm.loop only expected on branches", &I);
LoopInfo LI;
LI.analyze(DT);
Loop* L = LI.getLoopFor(BB);
Assert(L, "llvm.loop not in a loop", &I);
Assert(L->isLoopLatch(BB), "llvm.loop not in a latch", &I);
  }

Note that the above just was a simple test. We do not want to reinitialize 
LoopInfo for each found occurence of !llvm.loop.
Another problem is that the Verifier is used by lots of tools that currently do 
not link with LoopInfo from the Analysis code module.

So maybe this should go into the LoopVerifier pass instead (although I'm not 
sure if that is commonly used).
Or is there some other place where we can do it? Or some other way to do a 
similar check (without using LoopInfo)?

I can bring this up on llvm-dev, to get some more feedback on the idea of 
having a verifier for this, and how to do it properly.


https://reviews.llvm.org/D48721



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

[PATCH] D48721: Patch to fix pragma metadata for do-while loops

[Bjorn Pettersson via Phabricator via cfe-commits]

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

LGTM!


https://reviews.llvm.org/D48721



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

[PATCH] D48721: Patch to fix pragma metadata for do-while loops

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

In https://reviews.llvm.org/D48721#1157571, @deepak2427 wrote:

> @Bjorn, Thanks for reviewing and accepting the patch.
>
> Could you please advise on the next steps?
>  Would someone else commit this on my behalf or should I request for commit 
> access?
>
> Thanks,
>  Deepak Panickal


I can commit this for you, and then you should be able to close the bugzilla 
ticket. Include a reference to the SVN id that will be the result of my commit 
when closing the bugzilla ticket.


https://reviews.llvm.org/D48721



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

[PATCH] D48721: Patch to fix pragma metadata for do-while loops

[Bjorn Pettersson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL336717: Patch to fix pragma metadata for do-while loops 
(authored by bjope, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D48721?vs=154244&id=154865#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D48721

Files:
  cfe/trunk/lib/CodeGen/CGStmt.cpp
  cfe/trunk/test/CodeGen/pragma-do-while.cpp


Index: cfe/trunk/test/CodeGen/pragma-do-while.cpp
===
--- cfe/trunk/test/CodeGen/pragma-do-while.cpp
+++ cfe/trunk/test/CodeGen/pragma-do-while.cpp
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s
+
+// We expect to get a loop structure like this:
+//do.body:   ; preds = %do.cond, ...
+//  ...
+//  br label %do.cond
+//do.cond:   ; preds = %do.body
+//  ...
+//  br i1 %cmp, label %do.body, label %do.end
+//do.end:; preds = %do.cond
+//  ...
+//
+// Verify that the loop metadata only is put on the backedge.
+//
+// CHECK-NOT: llvm.loop
+// CHECK-LABEL: do.cond:
+// CHECK: br {{.*}}, label %do.body, label %do.end, !llvm.loop ![[LMD1:[0-9]+]]
+// CHECK-LABEL: do.end:
+// CHECK-NOT: llvm.loop
+// CHECK: ![[LMD1]] = distinct !{![[LMD1]], ![[LMD2:[0-9]+]]}
+// CHECK: ![[LMD2]] = !{!"llvm.loop.unroll.count", i32 4}
+
+int test(int a[], int n) {
+  int i = 0;
+  int sum = 0;
+
+#pragma unroll 4
+  do
+  {
+a[i] = a[i] + 1;
+sum = sum + a[i];
+i++;
+  } while (i < n);
+
+  return sum;
+}
Index: cfe/trunk/lib/CodeGen/CGStmt.cpp
===
--- cfe/trunk/lib/CodeGen/CGStmt.cpp
+++ cfe/trunk/lib/CodeGen/CGStmt.cpp
@@ -777,19 +777,19 @@
   // Emit the body of the loop.
   llvm::BasicBlock *LoopBody = createBasicBlock("do.body");
 
-  const SourceRange &R = S.getSourceRange();
-  LoopStack.push(LoopBody, CGM.getContext(), DoAttrs,
- SourceLocToDebugLoc(R.getBegin()),
- SourceLocToDebugLoc(R.getEnd()));
-
   EmitBlockWithFallThrough(LoopBody, &S);
   {
 RunCleanupsScope BodyScope(*this);
 EmitStmt(S.getBody());
   }
 
   EmitBlock(LoopCond.getBlock());
 
+  const SourceRange &R = S.getSourceRange();
+  LoopStack.push(LoopBody, CGM.getContext(), DoAttrs,
+ SourceLocToDebugLoc(R.getBegin()),
+ SourceLocToDebugLoc(R.getEnd()));
+
   // C99 6.8.5.2: "The evaluation of the controlling expression takes place
   // after each execution of the loop body."
 


Index: cfe/trunk/test/CodeGen/pragma-do-while.cpp
===
--- cfe/trunk/test/CodeGen/pragma-do-while.cpp
+++ cfe/trunk/test/CodeGen/pragma-do-while.cpp
@@ -0,0 +1,36 @@
+// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s
+
+// We expect to get a loop structure like this:
+//do.body:   ; preds = %do.cond, ...
+//  ...
+//  br label %do.cond
+//do.cond:   ; preds = %do.body
+//  ...
+//  br i1 %cmp, label %do.body, label %do.end
+//do.end:; preds = %do.cond
+//  ...
+//
+// Verify that the loop metadata only is put on the backedge.
+//
+// CHECK-NOT: llvm.loop
+// CHECK-LABEL: do.cond:
+// CHECK: br {{.*}}, label %do.body, label %do.end, !llvm.loop ![[LMD1:[0-9]+]]
+// CHECK-LABEL: do.end:
+// CHECK-NOT: llvm.loop
+// CHECK: ![[LMD1]] = distinct !{![[LMD1]], ![[LMD2:[0-9]+]]}
+// CHECK: ![[LMD2]] = !{!"llvm.loop.unroll.count", i32 4}
+
+int test(int a[], int n) {
+  int i = 0;
+  int sum = 0;
+
+#pragma unroll 4
+  do
+  {
+a[i] = a[i] + 1;
+sum = sum + a[i];
+i++;
+  } while (i < n);
+
+  return sum;
+}
Index: cfe/trunk/lib/CodeGen/CGStmt.cpp
===
--- cfe/trunk/lib/CodeGen/CGStmt.cpp
+++ cfe/trunk/lib/CodeGen/CGStmt.cpp
@@ -777,19 +777,19 @@
   // Emit the body of the loop.
   llvm::BasicBlock *LoopBody = createBasicBlock("do.body");
 
-  const SourceRange &R = S.getSourceRange();
-  LoopStack.push(LoopBody, CGM.getContext(), DoAttrs,
- SourceLocToDebugLoc(R.getBegin()),
- SourceLocToDebugLoc(R.getEnd()));
-
   EmitBlockWithFallThrough(LoopBody, &S);
   {
 RunCleanupsScope BodyScope(*this);
 EmitStmt(S.getBody());
   }
 
   EmitBlock(LoopCond.getBlock());
 
+  const SourceRange &R = S.getSourceRange();
+  LoopStack.push(LoopBody, CGM.getContext(), DoAttrs,
+ SourceLocToDebugLoc(R.getBegin()),
+ SourceLocToDebugLoc(R.getEnd()));
+
   // C99 6.8.5.2: "The evaluation of the controlling expression takes place
   // after each execution of the loop body."
 
___
cfe-commits mailin

[PATCH] D69261: Prune Pass.h include from DataLayout.h. NFCI

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope created this revision.
bjope added a reviewer: rnk.
Herald added subscribers: cfe-commits, hiraditya, mehdi_amini.
Herald added projects: clang, LLVM.

Reduce include dependencies by no longer including Pass.h from
DataLayout.h. That include seemed irrelevant to DataLayout, as
wee as lots of users of DataLayout.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69261

Files:
  clang/lib/Tooling/AllTUsExecution.cpp
  llvm/include/llvm/IR/DataLayout.h
  llvm/include/llvm/IR/Module.h
  llvm/include/llvm/IR/PassManager.h
  llvm/lib/Transforms/Utils/CanonicalizeAliases.cpp
  llvm/tools/llvm-profdata/llvm-profdata.cpp
  llvm/unittests/IR/ModuleTest.cpp


Index: llvm/unittests/IR/ModuleTest.cpp
===
--- llvm/unittests/IR/ModuleTest.cpp
+++ llvm/unittests/IR/ModuleTest.cpp
@@ -8,6 +8,7 @@
 
 #include "llvm/IR/Module.h"
 #include "llvm/IR/GlobalVariable.h"
+#include "llvm/Pass.h"
 #include "llvm/Support/RandomNumberGenerator.h"
 #include "gtest/gtest.h"
 
Index: llvm/tools/llvm-profdata/llvm-profdata.cpp
===
--- llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -26,6 +26,7 @@
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/Threading.h"
 #include "llvm/Support/ThreadPool.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
Index: llvm/lib/Transforms/Utils/CanonicalizeAliases.cpp
===
--- llvm/lib/Transforms/Utils/CanonicalizeAliases.cpp
+++ llvm/lib/Transforms/Utils/CanonicalizeAliases.cpp
@@ -33,6 +33,7 @@
 
 #include "llvm/IR/Operator.h"
 #include "llvm/IR/ValueHandle.h"
+#include "llvm/Pass.h"
 
 using namespace llvm;
 
Index: llvm/include/llvm/IR/PassManager.h
===
--- llvm/include/llvm/IR/PassManager.h
+++ llvm/include/llvm/IR/PassManager.h
@@ -45,6 +45,7 @@
 #include "llvm/IR/Module.h"
 #include "llvm/IR/PassInstrumentation.h"
 #include "llvm/IR/PassManagerInternal.h"
+#include "llvm/Pass.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/TypeName.h"
 #include "llvm/Support/raw_ostream.h"
Index: llvm/include/llvm/IR/Module.h
===
--- llvm/include/llvm/IR/Module.h
+++ llvm/include/llvm/IR/Module.h
@@ -46,6 +46,7 @@
 class GVMaterializer;
 class LLVMContext;
 class MemoryBuffer;
+class Pass;
 class RandomNumberGenerator;
 template  class SmallPtrSetImpl;
 class StructType;
Index: llvm/include/llvm/IR/DataLayout.h
===
--- llvm/include/llvm/IR/DataLayout.h
+++ llvm/include/llvm/IR/DataLayout.h
@@ -25,7 +25,6 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Type.h"
-#include "llvm/Pass.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MathExtras.h"
Index: clang/lib/Tooling/AllTUsExecution.cpp
===
--- clang/lib/Tooling/AllTUsExecution.cpp
+++ clang/lib/Tooling/AllTUsExecution.cpp
@@ -8,6 +8,7 @@
 
 #include "clang/Tooling/AllTUsExecution.h"
 #include "clang/Tooling/ToolExecutorPluginRegistry.h"
+#include "llvm/Support/Threading.h"
 #include "llvm/Support/ThreadPool.h"
 #include "llvm/Support/VirtualFileSystem.h"
 


Index: llvm/unittests/IR/ModuleTest.cpp
===
--- llvm/unittests/IR/ModuleTest.cpp
+++ llvm/unittests/IR/ModuleTest.cpp
@@ -8,6 +8,7 @@
 
 #include "llvm/IR/Module.h"
 #include "llvm/IR/GlobalVariable.h"
+#include "llvm/Pass.h"
 #include "llvm/Support/RandomNumberGenerator.h"
 #include "gtest/gtest.h"
 
Index: llvm/tools/llvm-profdata/llvm-profdata.cpp
===
--- llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -26,6 +26,7 @@
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/Threading.h"
 #include "llvm/Support/ThreadPool.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
Index: llvm/lib/Transforms/Utils/CanonicalizeAliases.cpp
===
--- llvm/lib/Transforms/Utils/CanonicalizeAliases.cpp
+++ llvm/lib/Transforms/Utils/CanonicalizeAliases.cpp
@@ -33,6 +33,7 @@
 
 #include "llvm/IR/Operator.h"
 #include "llvm/IR/ValueHandle.h"
+#include "llvm/Pass.h"
 
 using namespace llvm;
 
Index: llvm/include/llvm/IR/PassManager.h
===
--- llvm/include/llvm/IR/PassManager.h
+++ llvm/include/llvm/IR/PassManager.h
@@ -4

[PATCH] D69262: Prune include of DataLayout.h from include/clang/Basic/TargetInfo.h. NFC

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope created this revision.
bjope added a reviewer: rnk.
Herald added subscribers: cfe-commits, nhaehnle, jvesely.
Herald added a project: clang.

Use a forward declaration of DataLayout instead of including
DataLayout.h in clangs TargetInfo.h. This reduces include
dependencies toward DataLayout.h (and other headers such as
DerivedTypes.h, Type.h that is included by DataLayout.h).

Needed to move implemantation of TargetInfo::resetDataLayout
from TargetInfo.h to TargetInfo.cpp.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69262

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/TargetInfo.cpp
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/unittests/AST/DeclTest.cpp


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -16,6 +16,7 @@
 #include "clang/Basic/LLVM.h"
 #include "clang/Tooling/Tooling.h"
 #include "gtest/gtest.h"
+#include "llvm/IR/DataLayout.h"
 
 using namespace clang::ast_matchers;
 using namespace clang::tooling;
Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -24,6 +24,7 @@
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Serialization/ASTReader.h"
 #include "llvm/ADT/APFloat.h"
+#include "llvm/IR/DataLayout.h"
 using namespace clang;
 
 static bool MacroBodyEndsInBackslash(StringRef MacroBody) {
Index: clang/lib/Basic/Targets/AMDGPU.cpp
===
--- clang/lib/Basic/Targets/AMDGPU.cpp
+++ clang/lib/Basic/Targets/AMDGPU.cpp
@@ -17,6 +17,7 @@
 #include "clang/Basic/MacroBuilder.h"
 #include "clang/Basic/TargetBuiltins.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/IR/DataLayout.h"
 
 using namespace clang;
 using namespace clang::targets;
Index: clang/lib/Basic/TargetInfo.cpp
===
--- clang/lib/Basic/TargetInfo.cpp
+++ clang/lib/Basic/TargetInfo.cpp
@@ -17,6 +17,7 @@
 #include "clang/Basic/LangOptions.h"
 #include "llvm/ADT/APFloat.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/IR/DataLayout.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/TargetParser.h"
 #include 
@@ -136,6 +137,10 @@
 // Out of line virtual dtor for TargetInfo.
 TargetInfo::~TargetInfo() {}
 
+void TargetInfo::resetDataLayout(StringRef DL) {
+  DataLayout.reset(new llvm::DataLayout(DL));
+}
+
 bool
 TargetInfo::checkCFProtectionBranchSupported(DiagnosticsEngine &Diags) const {
   Diags.Report(diag::err_opt_not_valid_on_target) << "cf-protection=branch";
Index: clang/include/clang/Basic/TargetInfo.h
===
--- clang/include/clang/Basic/TargetInfo.h
+++ clang/include/clang/Basic/TargetInfo.h
@@ -19,14 +19,15 @@
 #include "clang/Basic/Specifiers.h"
 #include "clang/Basic/TargetCXXABI.h"
 #include "clang/Basic/TargetOptions.h"
+#include "llvm/ADT/APFloat.h"
 #include "llvm/ADT/APInt.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Triple.h"
-#include "llvm/IR/DataLayout.h"
 #include "llvm/Support/DataTypes.h"
 #include "llvm/Support/VersionTuple.h"
 #include 
@@ -35,6 +36,7 @@
 
 namespace llvm {
 struct fltSemantics;
+class DataLayout;
 }
 
 namespace clang {
@@ -198,9 +200,7 @@
   // TargetInfo Constructor.  Default initializes all fields.
   TargetInfo(const llvm::Triple &T);
 
-  void resetDataLayout(StringRef DL) {
-DataLayout.reset(new llvm::DataLayout(DL));
-  }
+  void resetDataLayout(StringRef DL);
 
 public:
   /// Construct a target for the given options.


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -16,6 +16,7 @@
 #include "clang/Basic/LLVM.h"
 #include "clang/Tooling/Tooling.h"
 #include "gtest/gtest.h"
+#include "llvm/IR/DataLayout.h"
 
 using namespace clang::ast_matchers;
 using namespace clang::tooling;
Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -24,6 +24,7 @@
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Serialization/ASTReader.h"
 #include "llvm/ADT/APFloat.h"
+#include "llvm/IR/DataLayout.h"
 using namespace clang;
 
 static bool MacroBodyEndsInBackslash(StringRef MacroBody) {
Index: clang/lib/Basic/Targets/AMDGPU.cpp
===
--- clang/lib/Basic/Targets/AMDGPU.cpp
+++ cl

[PATCH] D69261: Prune Pass.h include from DataLayout.h. NFCI

[Bjorn Pettersson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1f43ea41c330: Prune Pass.h include from DataLayout.h. NFCI 
(authored by bjope).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69261

Files:
  clang/lib/Tooling/AllTUsExecution.cpp
  llvm/include/llvm/IR/DataLayout.h
  llvm/include/llvm/IR/Module.h
  llvm/include/llvm/IR/PassManager.h
  llvm/lib/Transforms/Utils/CanonicalizeAliases.cpp
  llvm/tools/llvm-profdata/llvm-profdata.cpp
  llvm/unittests/IR/ModuleTest.cpp


Index: llvm/unittests/IR/ModuleTest.cpp
===
--- llvm/unittests/IR/ModuleTest.cpp
+++ llvm/unittests/IR/ModuleTest.cpp
@@ -8,6 +8,7 @@
 
 #include "llvm/IR/Module.h"
 #include "llvm/IR/GlobalVariable.h"
+#include "llvm/Pass.h"
 #include "llvm/Support/RandomNumberGenerator.h"
 #include "gtest/gtest.h"
 
Index: llvm/tools/llvm-profdata/llvm-profdata.cpp
===
--- llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -26,6 +26,7 @@
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/Threading.h"
 #include "llvm/Support/ThreadPool.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
Index: llvm/lib/Transforms/Utils/CanonicalizeAliases.cpp
===
--- llvm/lib/Transforms/Utils/CanonicalizeAliases.cpp
+++ llvm/lib/Transforms/Utils/CanonicalizeAliases.cpp
@@ -33,6 +33,7 @@
 
 #include "llvm/IR/Operator.h"
 #include "llvm/IR/ValueHandle.h"
+#include "llvm/Pass.h"
 
 using namespace llvm;
 
Index: llvm/include/llvm/IR/PassManager.h
===
--- llvm/include/llvm/IR/PassManager.h
+++ llvm/include/llvm/IR/PassManager.h
@@ -45,6 +45,7 @@
 #include "llvm/IR/Module.h"
 #include "llvm/IR/PassInstrumentation.h"
 #include "llvm/IR/PassManagerInternal.h"
+#include "llvm/Pass.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/TypeName.h"
 #include "llvm/Support/raw_ostream.h"
Index: llvm/include/llvm/IR/Module.h
===
--- llvm/include/llvm/IR/Module.h
+++ llvm/include/llvm/IR/Module.h
@@ -46,6 +46,7 @@
 class GVMaterializer;
 class LLVMContext;
 class MemoryBuffer;
+class Pass;
 class RandomNumberGenerator;
 template  class SmallPtrSetImpl;
 class StructType;
Index: llvm/include/llvm/IR/DataLayout.h
===
--- llvm/include/llvm/IR/DataLayout.h
+++ llvm/include/llvm/IR/DataLayout.h
@@ -25,7 +25,6 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Type.h"
-#include "llvm/Pass.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MathExtras.h"
Index: clang/lib/Tooling/AllTUsExecution.cpp
===
--- clang/lib/Tooling/AllTUsExecution.cpp
+++ clang/lib/Tooling/AllTUsExecution.cpp
@@ -8,6 +8,7 @@
 
 #include "clang/Tooling/AllTUsExecution.h"
 #include "clang/Tooling/ToolExecutorPluginRegistry.h"
+#include "llvm/Support/Threading.h"
 #include "llvm/Support/ThreadPool.h"
 #include "llvm/Support/VirtualFileSystem.h"
 


Index: llvm/unittests/IR/ModuleTest.cpp
===
--- llvm/unittests/IR/ModuleTest.cpp
+++ llvm/unittests/IR/ModuleTest.cpp
@@ -8,6 +8,7 @@
 
 #include "llvm/IR/Module.h"
 #include "llvm/IR/GlobalVariable.h"
+#include "llvm/Pass.h"
 #include "llvm/Support/RandomNumberGenerator.h"
 #include "gtest/gtest.h"
 
Index: llvm/tools/llvm-profdata/llvm-profdata.cpp
===
--- llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -26,6 +26,7 @@
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/Threading.h"
 #include "llvm/Support/ThreadPool.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
Index: llvm/lib/Transforms/Utils/CanonicalizeAliases.cpp
===
--- llvm/lib/Transforms/Utils/CanonicalizeAliases.cpp
+++ llvm/lib/Transforms/Utils/CanonicalizeAliases.cpp
@@ -33,6 +33,7 @@
 
 #include "llvm/IR/Operator.h"
 #include "llvm/IR/ValueHandle.h"
+#include "llvm/Pass.h"
 
 using namespace llvm;
 
Index: llvm/include/llvm/IR/PassManager.h
===
--- llvm/include/llvm/IR/PassManager.h
+++ llvm/include/llvm/IR/PassManager.h
@@ -45,6 +45,7 @@
 #include "llvm/IR/Module.h"
 #include "llvm/IR/PassInstrumentation.

[PATCH] D69262: Prune include of DataLayout.h from include/clang/Basic/TargetInfo.h. NFC

[Bjorn Pettersson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG78424e5f8417: Prune include of DataLayout.h from 
include/clang/Basic/TargetInfo.h. NFC (authored by bjope).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69262

Files:
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/TargetInfo.cpp
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/Frontend/InitPreprocessor.cpp
  clang/unittests/AST/DeclTest.cpp


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -16,6 +16,7 @@
 #include "clang/Basic/LLVM.h"
 #include "clang/Tooling/Tooling.h"
 #include "gtest/gtest.h"
+#include "llvm/IR/DataLayout.h"
 
 using namespace clang::ast_matchers;
 using namespace clang::tooling;
Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -24,6 +24,7 @@
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Serialization/ASTReader.h"
 #include "llvm/ADT/APFloat.h"
+#include "llvm/IR/DataLayout.h"
 using namespace clang;
 
 static bool MacroBodyEndsInBackslash(StringRef MacroBody) {
Index: clang/lib/Basic/Targets/AMDGPU.cpp
===
--- clang/lib/Basic/Targets/AMDGPU.cpp
+++ clang/lib/Basic/Targets/AMDGPU.cpp
@@ -17,6 +17,7 @@
 #include "clang/Basic/MacroBuilder.h"
 #include "clang/Basic/TargetBuiltins.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/IR/DataLayout.h"
 
 using namespace clang;
 using namespace clang::targets;
Index: clang/lib/Basic/TargetInfo.cpp
===
--- clang/lib/Basic/TargetInfo.cpp
+++ clang/lib/Basic/TargetInfo.cpp
@@ -17,6 +17,7 @@
 #include "clang/Basic/LangOptions.h"
 #include "llvm/ADT/APFloat.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/IR/DataLayout.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/TargetParser.h"
 #include 
@@ -136,6 +137,10 @@
 // Out of line virtual dtor for TargetInfo.
 TargetInfo::~TargetInfo() {}
 
+void TargetInfo::resetDataLayout(StringRef DL) {
+  DataLayout.reset(new llvm::DataLayout(DL));
+}
+
 bool
 TargetInfo::checkCFProtectionBranchSupported(DiagnosticsEngine &Diags) const {
   Diags.Report(diag::err_opt_not_valid_on_target) << "cf-protection=branch";
Index: clang/include/clang/Basic/TargetInfo.h
===
--- clang/include/clang/Basic/TargetInfo.h
+++ clang/include/clang/Basic/TargetInfo.h
@@ -19,14 +19,15 @@
 #include "clang/Basic/Specifiers.h"
 #include "clang/Basic/TargetCXXABI.h"
 #include "clang/Basic/TargetOptions.h"
+#include "llvm/ADT/APFloat.h"
 #include "llvm/ADT/APInt.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Triple.h"
-#include "llvm/IR/DataLayout.h"
 #include "llvm/Support/DataTypes.h"
 #include "llvm/Support/VersionTuple.h"
 #include 
@@ -35,6 +36,7 @@
 
 namespace llvm {
 struct fltSemantics;
+class DataLayout;
 }
 
 namespace clang {
@@ -198,9 +200,7 @@
   // TargetInfo Constructor.  Default initializes all fields.
   TargetInfo(const llvm::Triple &T);
 
-  void resetDataLayout(StringRef DL) {
-DataLayout.reset(new llvm::DataLayout(DL));
-  }
+  void resetDataLayout(StringRef DL);
 
 public:
   /// Construct a target for the given options.


Index: clang/unittests/AST/DeclTest.cpp
===
--- clang/unittests/AST/DeclTest.cpp
+++ clang/unittests/AST/DeclTest.cpp
@@ -16,6 +16,7 @@
 #include "clang/Basic/LLVM.h"
 #include "clang/Tooling/Tooling.h"
 #include "gtest/gtest.h"
+#include "llvm/IR/DataLayout.h"
 
 using namespace clang::ast_matchers;
 using namespace clang::tooling;
Index: clang/lib/Frontend/InitPreprocessor.cpp
===
--- clang/lib/Frontend/InitPreprocessor.cpp
+++ clang/lib/Frontend/InitPreprocessor.cpp
@@ -24,6 +24,7 @@
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Serialization/ASTReader.h"
 #include "llvm/ADT/APFloat.h"
+#include "llvm/IR/DataLayout.h"
 using namespace clang;
 
 static bool MacroBodyEndsInBackslash(StringRef MacroBody) {
Index: clang/lib/Basic/Targets/AMDGPU.cpp
===
--- clang/lib/Basic/Targets/AMDGPU.cpp
+++ clang/lib/Basic/Targets/AMDGPU.cpp
@@ -17,6 +17,7 @@
 #include "clang/Basic/MacroBuilder.h"
 #include "clang/Basic/TargetBuiltins.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/IR/DataLayout.h"
 
 using namespac

[PATCH] D62922: [WebAssembly] Implement "Reactor" mode

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang/test/Driver/wasm-toolchain.c:116
+// CHECK-COMMAND: clang{{.*}}" "-cc1" {{.*}} "-o" "[[temp:[^"]*]]"
+// CHECK-COMMAND: wasm-ld{{.*}}" "crt1.o" "[[temp]]" "-lc" 
"{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"
+

This isn't working for me on redhat 7 servers. I get
```
 "wasm-ld" "-L/lib" "/lib/crt1.o" ...
```
No idea why I get the full patch to crt1.o. But it's not matching with the 
FileCheck pattern.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62922



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

[PATCH] D62922: [WebAssembly] Implement "Reactor" mode

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang/test/Driver/wasm-toolchain.c:116
+// CHECK-COMMAND: clang{{.*}}" "-cc1" {{.*}} "-o" "[[temp:[^"]*]]"
+// CHECK-COMMAND: wasm-ld{{.*}}" "crt1.o" "[[temp]]" "-lc" 
"{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"
+

bjope wrote:
> This isn't working for me on redhat 7 servers. I get
> ```
>  "wasm-ld" "-L/lib" "/lib/crt1.o" ...
> ```
> No idea why I get the full patch to crt1.o. But it's not matching with the 
> FileCheck pattern.
/full patch/full path/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62922



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

[PATCH] D62922: [WebAssembly] Implement "Reactor" mode

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang/test/Driver/wasm-toolchain.c:116
+// CHECK-COMMAND: clang{{.*}}" "-cc1" {{.*}} "-o" "[[temp:[^"]*]]"
+// CHECK-COMMAND: wasm-ld{{.*}}" "crt1.o" "[[temp]]" "-lc" 
"{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"
+

sunfish wrote:
> bjope wrote:
> > bjope wrote:
> > > This isn't working for me on redhat 7 servers. I get
> > > ```
> > >  "wasm-ld" "-L/lib" "/lib/crt1.o" ...
> > > ```
> > > No idea why I get the full patch to crt1.o. But it's not matching with 
> > > the FileCheck pattern.
> > /full patch/full path/
> Would you be able to test whether the following patch fixes it?
> 
> ```diff
> diff --git a/clang/test/Driver/wasm-toolchain.c 
> b/clang/test/Driver/wasm-toolchain.c
> index 8300a81614e..332e6048cc5 100644
> --- a/clang/test/Driver/wasm-toolchain.c
> +++ b/clang/test/Driver/wasm-toolchain.c
> @@ -110,11 +110,11 @@
> 
>  // Basic exec-model tests.
> 
> -// RUN: %clang %s -### -no-canonical-prefixes -target wasm32-unknown-unknown 
> -mexec-model=command 2>&1 \
> +// RUN: %clang %s -### -no-canonical-prefixes -target wasm32-unknown-unknown 
> --sysroot=%s/no-sysroot-there -mexec-model=command 2>&1 \
>  // RUN:   | FileCheck -check-prefix=CHECK-COMMAND %s
>  // CHECK-COMMAND: clang{{.*}}" "-cc1" {{.*}} "-o" "[[temp:[^"]*]]"
>  // CHECK-COMMAND: wasm-ld{{.*}}" "crt1.o" "[[temp]]" "-lc" 
> "{{.*[/\\]}}libclang_rt.builtins-wasm32.a" "-o" "a.out"
> 
> -// RUN: %clang %s -### -no-canonical-prefixes -target wasm32-unknown-unknown 
> -mexec-model=reactor 2>&1 \
> +// RUN: %clang %s -### -no-canonical-prefixes -target wasm32-unknown-unknown 
> --sysroot=%s/no-sysroot-there -mexec-model=reactor 2>&1 \
>  // RUN:   | FileCheck -check-prefix=CHECK-REACTOR %s
>  // CHECK-REACTOR: wasm-ld{{.*}}" {{.*}} "--entry" "_initialize" {{.*}}
> ```
Yes, that is working.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62922



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

[PATCH] D81920: [clangd] Change FSProvider::getFileSystem to take CurrentWorkingDirectory

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

I think the warnings can be hidden by something like this (I'm no expert in 
this area though, but it seems like this technique has been used a number of 
times before in llvm, so hopefully it applies here as well):

  diff --git a/clang-tools-extra/clangd/Preamble.cpp 
b/clang-tools-extra/clangd/Preamble.cpp
  index ca0a76db78f4..1970541bc56a 100644
  --- a/clang-tools-extra/clangd/Preamble.cpp
  +++ b/clang-tools-extra/clangd/Preamble.cpp
  @@ -230,6 +230,7 @@ llvm::Expected
   scanPreamble(llvm::StringRef Contents, const tooling::CompileCommand &Cmd) {
 class EmptyFS : public ThreadsafeFS {
 public:
  +using ThreadsafeFS::view;
   llvm::IntrusiveRefCntPtr
   view(llvm::NoneType) const override {
 return new llvm::vfs::InMemoryFileSystem;
  diff --git a/clang-tools-extra/clangd/support/ThreadsafeFS.h 
b/clang-tools-extra/clangd/support/ThreadsafeFS.h
  index aa6825fb3999..eb9016bad201 100644
  --- a/clang-tools-extra/clangd/support/ThreadsafeFS.h
  +++ b/clang-tools-extra/clangd/support/ThreadsafeFS.h
  @@ -42,6 +42,7 @@ public:
   
   class RealThreadsafeFS : public ThreadsafeFS {
   public:
  +  using ThreadsafeFS::view;
 llvm::IntrusiveRefCntPtr
 view(llvm::NoneType) const override;
   };

at least I get no warnings when building clangd (haven't checked the unittests, 
but I don't care about warnings in those right now).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81920



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

[PATCH] D85157: [Sema] Add casting check for integer to fixed point conversions

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang/lib/Sema/SemaCast.cpp:2660
 return;
+  if (SrcType->isIntegerType() && DestType->isFixedPointType())
+return;

Is this really the intention with the patch?

It does match the "summary" above, but isn't the warning well deserved for 
int->fixed cast.

I also base that question on the you referred to this patch from our bugtracker 
at Ericsson, which was about conversions from `long __fixed` to `long __fixed`. 
So I kind of expected to find `(SrcType->isFixedPointType() && 
DestType->isFixedPointType())` here (similar to the checks above that avoids 
warning when SrcType equals the DestType).

The test case I expected (related to our downstream bug report) would look like 
this:

```
_Fract ff1(void);

void
foo(void)
{
  (_Fract)ff1();  // No warning expected.
}
```




Comment at: clang/test/Sema/warn-bad-function-cast.c:2
 // RUN: %clang_cc1 %s -fsyntax-only -Wno-unused-value -Wbad-function-cast 
-triple x86_64-unknown-unknown -verify
+// RUN: %clang_cc1 %s -fsyntax-only -Wno-unused-value -Wbad-function-cast 
-DFIXED_POINT -ffixed-point -triple x86_64-unknown-unknown -verify
 // rdar://9103192

I doubt it is worth running the whole test twice just to test both with and 
without `-ffixed-point`, specially since there are no differences in the 
earlier tests when enabling `-ffixed-point`. So just adding `-ffixed-point` to 
the pre-existing RUN-line would save us from running this test case a humongous 
number of extra times during the next decade.



Comment at: clang/test/Sema/warn-bad-function-cast.c:49
+#ifdef FIXED_POINT
+  (void)(_Fract) if1(); // no warning
+#endif

This should be added before the line saying `/* All following casts issue 
warning */`.



Comment at: clang/test/Sema/warn-bad-function-cast.c:49
+#ifdef FIXED_POINT
+  (void)(_Fract) if1(); // no warning
+#endif

bjope wrote:
> This should be added before the line saying `/* All following casts issue 
> warning */`.
Is the `(void)` needed/relevant here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85157

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

[PATCH] D85157: [Sema] Add casting check for integer to fixed point conversions

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang/test/Sema/warn-bad-function-cast.c:49
+#ifdef FIXED_POINT
+  (void)(_Fract) if1(); // no warning
+#endif

bjope wrote:
> bjope wrote:
> > This should be added before the line saying `/* All following casts issue 
> > warning */`.
> Is the `(void)` needed/relevant here?
As questioned earlier, shouldn't we expect a warning for this scenario?

There is however a problem that we get the warning for _Fract to _Fract 
conversion. And it would be nice with a more complete set of tests involving 
both FixedPoint->FixedPoint, FixedPoint->Integer and Integer->FixedPoint casts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85157

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

[PATCH] D85157: [Sema] Add casting check for integer to fixed point conversions

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang/lib/Sema/SemaCast.cpp:2660
 return;
+  if (SrcType->isIntegerType() && DestType->isFixedPointType())
+return;

vabridgers wrote:
> bjope wrote:
> > Is this really the intention with the patch?
> > 
> > It does match the "summary" above, but isn't the warning well deserved for 
> > int->fixed cast.
> > 
> > I also base that question on the you referred to this patch from our 
> > bugtracker at Ericsson, which was about conversions from `long __fixed` to 
> > `long __fixed`. So I kind of expected to find `(SrcType->isFixedPointType() 
> > && DestType->isFixedPointType())` here (similar to the checks above that 
> > avoids warning when SrcType equals the DestType).
> > 
> > The test case I expected (related to our downstream bug report) would look 
> > like this:
> > 
> > ```
> > _Fract ff1(void);
> > 
> > void
> > foo(void)
> > {
> >   (_Fract)ff1();  // No warning expected.
> > }
> > ```
> > 
> I improve the commit message detail. Please read and let me know if the 
> intent is still not clear. 
I'm afraid you didn't update the summary in phabricator? (I've never managed to 
do that using arcanist myself, after having updated the commit msg locally I 
usually need to update the description in phabricator using the web ui).



Comment at: clang/lib/Sema/SemaCast.cpp:2660
 return;
+  if (SrcType->isIntegerType() && DestType->isFixedPointType())
+return;

bjope wrote:
> vabridgers wrote:
> > bjope wrote:
> > > Is this really the intention with the patch?
> > > 
> > > It does match the "summary" above, but isn't the warning well deserved 
> > > for int->fixed cast.
> > > 
> > > I also base that question on the you referred to this patch from our 
> > > bugtracker at Ericsson, which was about conversions from `long __fixed` 
> > > to `long __fixed`. So I kind of expected to find 
> > > `(SrcType->isFixedPointType() && DestType->isFixedPointType())` here 
> > > (similar to the checks above that avoids warning when SrcType equals the 
> > > DestType).
> > > 
> > > The test case I expected (related to our downstream bug report) would 
> > > look like this:
> > > 
> > > ```
> > > _Fract ff1(void);
> > > 
> > > void
> > > foo(void)
> > > {
> > >   (_Fract)ff1();  // No warning expected.
> > > }
> > > ```
> > > 
> > I improve the commit message detail. Please read and let me know if the 
> > intent is still not clear. 
> I'm afraid you didn't update the summary in phabricator? (I've never managed 
> to do that using arcanist myself, after having updated the commit msg locally 
> I usually need to update the description in phabricator using the web ui).
> Is this really the intention with the patch?
> 
> It does match the "summary" above, but isn't the warning well deserved for 
> int->fixed cast.
> 
> I also base that question on the you referred to this patch from our 
> bugtracker at Ericsson, which was about conversions from `long __fixed` to 
> `long __fixed`. So I kind of expected to find `(SrcType->isFixedPointType() 
> && DestType->isFixedPointType())` here (similar to the checks above that 
> avoids warning when SrcType equals the DestType).
> 
> The test case I expected (related to our downstream bug report) would look 
> like this:
> 
> ```
> _Fract ff1(void);
> 
> void
> foo(void)
> {
>   (_Fract)ff1();  // No warning expected.
> }
> ```
> 





Comment at: clang/lib/Sema/SemaCast.cpp:2660
 return;
+  if (SrcType->isIntegerType() && DestType->isFixedPointType())
+return;

bjope wrote:
> bjope wrote:
> > vabridgers wrote:
> > > bjope wrote:
> > > > Is this really the intention with the patch?
> > > > 
> > > > It does match the "summary" above, but isn't the warning well deserved 
> > > > for int->fixed cast.
> > > > 
> > > > I also base that question on the you referred to this patch from our 
> > > > bugtracker at Ericsson, which was about conversions from `long __fixed` 
> > > > to `long __fixed`. So I kind of expected to find 
> > > > `(SrcType->isFixedPointType() && DestType->isFixedPointType())` here 
> > > > (similar to the checks above that avoids warning when SrcType equals 
> > > > the DestType).
> > > > 
> > > > The test case I expected (related to our downstream bug report) would 
> > > > look like this:
> > > > 
> > > > ```
> > > > _Fract ff1(void);
> > > > 
> > > > void
> > > > foo(void)
> > > > {
> > > >   (_Fract)ff1();  // No warning expected.
> > > > }
> > > > ```
> > > > 
> > > I improve the commit message detail. Please read and let me know if the 
> > > intent is still not clear. 
> > I'm afraid you didn't update the summary in phabricator? (I've never 
> > managed to do that using arcanist myself, after having updated the commit 
> > msg locally I usually need to update the description in phabricator using 
> > the web ui).
> > Is this really the intention with the patch?
> > 
> > It does match the "summary" above, but i

[PATCH] D85191: [AST] Get field size in chars rather than bits in RecordLayoutBuilder.

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

In D85191#2196863 , @rsmith wrote:

> In D85191#2195923 , @ebevhan wrote:
>
>> In D85191#2193645 , @rsmith wrote:
>>
 This is not ideal, since it makes the calculations char size dependent, 
 and breaks for sizes that are not a multiple of the char size.
>>>
>>> How can we have a non-bitfield member whose size is not a multiple of the 
>>> size of a char?
>>
>> Downstream, we have fixed-point types that are 24 bits large, but where the 
>> char size is 16. The type then takes up 2 chars, where 8 of the bits are 
>> padding. The only way in Clang to express that the width of the bit 
>> representation of a type should be smaller than the number of chars it takes 
>> up in memory -- and consequently, produce an `i24` in IR -- is to return a 
>> non-charsize multiple from getTypeInfo.
>
> This violates the C and C++ language rules, which require the size of every 
> type to be a multiple of the size of char. A type with 24 value bits and 8 
> padding bits should report a type size of 32 bits, just like `bool` reports a 
> size of `CHAR_BIT` bits despite having only 1 value bit, and x86_64 `long 
> double` reports a type size of 128 bits despite having only 80 value bits.

I don't see that it breaks the language rules. The sizeof result for the 24 bit 
type should be 2 in the target described by @ebevhan  (two 16-bit bytes). But I 
imagine that without this patch it is reported as 24/16=1, right?

So isn't the problem that toCharUnitsFromBits is rounding down when given a 
bitsize that isn't a multiple of CHAR_BIT? Would it perhaps make sense to let 
it round up instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85191

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

[PATCH] D85157: [Sema] Add casting check for fixed to fixed point conversions

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang/test/Sema/warn-bad-function-cast.c:49
+#ifdef FIXED_POINT
+  (void)(_Fract) if1(); // no warning
+#endif

bjope wrote:
> vabridgers wrote:
> > bjope wrote:
> > > bjope wrote:
> > > > bjope wrote:
> > > > > This should be added before the line saying `/* All following casts 
> > > > > issue warning */`.
> > > > Is the `(void)` needed/relevant here?
> > > As questioned earlier, shouldn't we expect a warning for this scenario?
> > > 
> > > There is however a problem that we get the warning for _Fract to _Fract 
> > > conversion. And it would be nice with a more complete set of tests 
> > > involving both FixedPoint->FixedPoint, FixedPoint->Integer and 
> > > Integer->FixedPoint casts.
> > If you have any *specific* suggestions for test cases, I'm open to that.
> Add:
> 
> ```
> _Fract ff1(void);
> ```
> 
> And inside foo add these three tests (you'll need to add appropriate expects):
> ```
> (_Fract)ff1();  // No warning expected.
> (_Fract)if1();  // Warning expected.
> (int)ff1();  // Warning expected.
> ```
I still think it would be nice not to break the structure of this test. Tests 
seem to be divided into three categories:

/* Casts to void types are always OK.  */

  /* Casts to the same type or similar types are OK.  */

/* All following casts issue warning */

And you have currently inserted all new tests in the last section.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85157

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

[PATCH] D85157: [Sema] Add casting check for fixed to fixed point conversions

[Bjorn Pettersson via Phabricator via cfe-commits]

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

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85157

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

[PATCH] D80525: [clangd] Fix crash-bug in preamble indexing when using modules.

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

In D80525#2215641 , @adamcz wrote:

> This is quite strange. I can't reproduce this with gcc 7.5.0, 9.3.0 or 
> 10.0.1. Which version are you using?
>
> There are bugs around clangd + modules + diagnostics. I'm fixing one in 
> https://reviews.llvm.org/D85753 (not in it's final form yet), but it doesn't 
> seem to be the same issue (can't be sure, but I think this is using the right 
> SourceManager). This test is not supposed to generate any diagnostics, so 
> that's the really surprising bit. Are you building from clean HEAD, or do you 
> have any local modifications that could generate a extra diagnostics in this 
> test?
>
> The other option is that the failure is caused by module_cache_path. This 
> test, unlike other clangd tests, needs real file system (to write the .pcm 
> file and then read it back) and perhaps that doesn't work in your jenkins 
> setup.
>
> Are there any special options or things unique to your setup I could try out 
> to reproduce this?

We actually have had problems reproducing it outside jenkins ourselves as well. 
That is kind of why we were a bit clueless about what to do and what might be 
causing it.

Are those `VFS: failed to set CWD to /clangd-test: No such file or directory` 
outputs expected?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80525

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

[PATCH] D80525: [clangd] Fix crash-bug in preamble indexing when using modules.

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

Might be that it is some other commit that cause the problem. Looking a bit 
closer at the jenkins history it seems like the tests passed once after this 
patch was added. But the next time it failed.

That second time the "[Diagnoostics] Reworked -Wstring-concatenation" commit 
had been pushed as well (commit b9af72bffe 
). And I 
see that there are some string concatenation going on in the test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80525

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

[PATCH] D80525: [clangd] Fix crash-bug in preamble indexing when using modules.

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

In D80525#2215778 , @adamcz wrote:

> Looks like it's an issue with not clearing the module cache. I'll revert this 
> change to fix the build, then re-submit with proper fix.

Ok, thanks. Let me know if you want me to investigate further on my side (such 
as adding more logging in those jenkins jobs).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80525

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

[PATCH] D132851: Further update -Wbitfield-constant-conversion for 1-bit bitfield

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang/test/Sema/constant-conversion.c:30
+  s.b = 1; // one-bit-warning {{implicit truncation from 'int' to a 
one-bit wide bit-field changes value from 1 to -1}}
+  s.b = true;  // no-warning (we suppress it manually to reduce false 
positives)
+  s.b = false; // no-warning

(Sorry for being late to the party, with post commit comments. I'm just trying 
to understand the reasoning about always suppressing the warning based on 
"true".)

Isn't the code a bit suspicious when using true/false from stdbool.h, while 
using a signed bitfield?

When doing things like 
```
(s.b == true) ? 1 : 0
```
the result would always be zero if s.b is a signed 1-bit bitfield.

So wouldn't it make more sense to actually use an unsigned bitfield (such as 
the bool type from stdbool.h) when the intent is to store a boolean value and 
using defines from stdbool.h?

Is perhaps the idea that we will get warnings about `(s.b == true)` being 
redundant in situations like this, and then we do not need a warning on the 
bitfield assignment? Such a reasoning would actually make some sense, since 
`(s.b == true)` never would be true even when the bitfield is assigned a 
non-constant value, so we can't rely on catching the problem by only looking at 
bitfield assignments involving true/false.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132851

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

[PATCH] D132851: Further update -Wbitfield-constant-conversion for 1-bit bitfield

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang/test/Sema/constant-conversion.c:30
+  s.b = 1; // one-bit-warning {{implicit truncation from 'int' to a 
one-bit wide bit-field changes value from 1 to -1}}
+  s.b = true;  // no-warning (we suppress it manually to reduce false 
positives)
+  s.b = false; // no-warning

aaron.ballman wrote:
> bjope wrote:
> > (Sorry for being late to the party, with post commit comments. I'm just 
> > trying to understand the reasoning about always suppressing the warning 
> > based on "true".)
> > 
> > Isn't the code a bit suspicious when using true/false from stdbool.h, while 
> > using a signed bitfield?
> > 
> > When doing things like 
> > ```
> > (s.b == true) ? 1 : 0
> > ```
> > the result would always be zero if s.b is a signed 1-bit bitfield.
> > 
> > So wouldn't it make more sense to actually use an unsigned bitfield (such 
> > as the bool type from stdbool.h) when the intent is to store a boolean 
> > value and using defines from stdbool.h?
> > 
> > Is perhaps the idea that we will get warnings about `(s.b == true)` being 
> > redundant in situations like this, and then we do not need a warning on the 
> > bitfield assignment? Such a reasoning would actually make some sense, since 
> > `(s.b == true)` never would be true even when the bitfield is assigned a 
> > non-constant value, so we can't rely on catching the problem by only 
> > looking at bitfield assignments involving true/false.
> > Isn't the code a bit suspicious when using true/false from stdbool.h, while 
> > using a signed bitfield?
> 
> Yes and no...
> 
> > When doing things like
> > ```
> > (s.b == true) ? 1 : 0
> > ```
> > the result would always be zero if s.b is a signed 1-bit bitfield.
> 
> You're correct, but that's a bit of a code smell because of `== true`.
> 
> > So wouldn't it make more sense to actually use an unsigned bitfield (such 
> > as the bool type from stdbool.h) when the intent is to store a boolean 
> > value and using defines from stdbool.h?
> 
> Yes, ideally.
> 
> > Is perhaps the idea that we will get warnings about (s.b == true) being 
> > redundant in situations like this, and then we do not need a warning on the 
> > bitfield assignment? Such a reasoning would actually make some sense, since 
> > (s.b == true) never would be true even when the bitfield is assigned a 
> > non-constant value, so we can't rely on catching the problem by only 
> > looking at bitfield assignments involving true/false.
> 
> The idea is more that someone using `true` is more likely to be treating the 
> field as a boolean and so they won't be comparing the bit-field against a 
> specific value, but instead testing it for its boolean value. This also 
> unifies the behavior between C and C++:  https://godbolt.org/z/WKP4xcPzT
> 
> We don't currently issue a diagnostic on `== true`, but that sure seems like 
> something `-Wtautological-compare` should pick up on (for bit-fields in 
> general, not just for one-bit bit-fields): https://godbolt.org/z/Tj4711Ysc
> 
> (Note, godbolt seems to have a Clang trunk that's ~8 days old, so diagnostic 
> behavior doesn't match actual trunk yet. That caught me by surprise.)
Ok, thanks! Sounds reasonable to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132851

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

[PATCH] D128472: [pseudo] Check follow-sets instead of tying reduce actions to lookahead tokens.

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

Our downstream bots have failed when using `LLVM_ENABLE_EXPENSIVE_CHECKS=ON` , 
and I think (unless my bisecting got screwed up) that it started to happen 
after this patch.

We typically get

  Failed Tests (10):
ClangPseudo :: lr-build-basic.test
ClangPseudo :: lr-build-conflicts.test
clangPseudo Unit Tests :: ./ClangPseudoTests/10/31
clangPseudo Unit Tests :: ./ClangPseudoTests/11/31
clangPseudo Unit Tests :: ./ClangPseudoTests/12/31
clangPseudo Unit Tests :: ./ClangPseudoTests/13/31
clangPseudo Unit Tests :: ./ClangPseudoTests/14/31
clangPseudo Unit Tests :: ./ClangPseudoTests/15/31
clangPseudo Unit Tests :: ./ClangPseudoTests/16/31
clangPseudo Unit Tests :: ./ClangPseudoTests/9/31

And the failing stack traces look like this:

  
/lib/gcc/x86_64-pc-linux-gnu/9.3.0/../../../../include/c++/9.3.0/bits/stl_vector.h:1060:
 std::vector::const_reference std::vector >::operator[](std::vector::size_type) const [_Tp 
= unsigned short, _Alloc = std::allocator]: Assertion 
'__builtin_expect(__n < this->size(), true)' failed.
   #0 0x0047b6b3 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) 
(/repo/llvm/build-all-expensive/tools/clang/tools/extra/pseudo/unittests/./ClangPseudoTests+0x47b6b3)
   #1 0x004795cc llvm::sys::RunSignalHandlers() 
(/repo/llvm/build-all-expensive/tools/clang/tools/extra/pseudo/unittests/./ClangPseudoTests+0x4795cc)
   #2 0x0047bcc6 SignalHandler(int) 
(/repo/llvm/build-all-expensive/tools/clang/tools/extra/pseudo/unittests/./ClangPseudoTests+0x47bcc6)
   #3 0x7f9ace5af630 __restore_rt (/lib64/libpthread.so.0+0xf630)
   #4 0x7f9acd6ce387 __GI_raise (/lib64/libc.so.6+0x36387)
   #5 0x7f9acd6cfa78 __GI_abort (/lib64/libc.so.6+0x37a78)
   #6 0x0045b988 
(/repo/llvm/build-all-expensive/tools/clang/tools/extra/pseudo/unittests/./ClangPseudoTests+0x45b988)
   #7 0x004e42a6 clang::pseudo::(anonymous 
namespace)::GLRReduce::popPending() 
(/repo/llvm/build-all-expensive/tools/clang/tools/extra/pseudo/unittests/./ClangPseudoTests+0x4e42a6)
   #8 0x004e15f2 clang::pseudo::(anonymous 
namespace)::GLRReduce::operator()(std::vector >&, unsigned short) 
(/repo/llvm/build-all-expensive/tools/clang/tools/extra/pseudo/unittests/./ClangPseudoTests+0x4e15f2)
   #9 0x004e2e15 
clang::pseudo::glrReduce(std::vector >&, unsigned short, 
clang::pseudo::ParseParams const&) 
(/repo/llvm/build-all-expensive/tools/clang/tools/extra/pseudo/unittests/./ClangPseudoTests+0x4e2e15)
  #10 0x00432379 clang::pseudo::(anonymous 
namespace)::GLRTest_ReduceConflictsSplitting_Test::TestBody() 
(/repo/llvm/build-all-expensive/tools/clang/tools/extra/pseudo/unittests/./ClangPseudoTests+0x432379)
  #11 0x004b8b6c testing::Test::Run() 
(/repo/llvm/build-all-expensive/tools/clang/tools/extra/pseudo/unittests/./ClangPseudoTests+0x4b8b6c)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128472

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

[PATCH] D128472: [pseudo] Check follow-sets instead of tying reduce actions to lookahead tokens.

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

In D128472#3625584 , @hokein wrote:

> Thanks for the report. There is an out-of-bound issue in 
> LRTable::getReduceRules, fixed in c99827349927a44334f2b04139168efd0bc87cd3 
> .

Great, thanks for the fix!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128472

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

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

Have had problems with failing stage 2 builds since this patch:

  FAILED: 
tools/clang/tools/extra/pseudo/unittests/CMakeFiles/ClangPseudoTests.dir/DirectiveMapTest.cpp.o
 
  /repo//install/stage2/bin/clang++  -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE 
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS 
-Itools/clang/tools/extra/pseudo/unittests 
-I/repo//llvm-project/clang-tools-extra/pseudo/unittests 
-I/repo//llvm-project/clang/include -Itools/clang/include -Iinclude 
-I/repo//llvm-project/llvm/include 
-I/repo//llvm-project/clang-tools-extra/pseudo/include 
-Itools/clang/tools/extra/pseudo/include -fPIC -fno-semantic-interposition 
-fvisibility-inlines-hidden -Werror -Werror=date-time 
-Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter 
-Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic 
-Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough 
-Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor 
-Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion 
-Wmisleading-indentation -fdiagnostics-color -ffunction-sections 
-fdata-sections -flto=thin -fno-common -Woverloaded-virtual 
-Wno-nested-anon-types -O3 -DNDEBUG-Wno-variadic-macros 
-Wno-gnu-zero-variadic-macro-arguments -fno-exceptions -fno-rtti -UNDEBUG 
-std=c++14 -MD -MT 
tools/clang/tools/extra/pseudo/unittests/CMakeFiles/ClangPseudoTests.dir/DirectiveMapTest.cpp.o
 -MF 
tools/clang/tools/extra/pseudo/unittests/CMakeFiles/ClangPseudoTests.dir/DirectiveMapTest.cpp.o.d
 -o 
tools/clang/tools/extra/pseudo/unittests/CMakeFiles/ClangPseudoTests.dir/DirectiveMapTest.cpp.o
 -c /repo//llvm-project/clang-tools-extra/pseudo/unittests/DirectiveMapTest.cpp
  
/repo//llvm-project/clang-tools-extra/pseudo/unittests/DirectiveMapTest.cpp:16:10:
 fatal error: 'gmock/gmock.h' file not found
  #include "gmock/gmock.h"
   ^~~
  1 error generated.

Is there some dependency to googletest missing somewhere?

(Unless someone not just knows about these things, then I guess I need to debug 
our build setup a bit more to give more details about it.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121233

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

[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

In D121233#3426155 , @sammccall wrote:

> In D121233#3425793 , @bjope wrote:
>
>> Have had problems with failing stage 2 builds since this patch:
>
> Should be fixed in 72ae6cc3a689869dcc15cfa8d0abcdd35a35a484 
> , please 
> let me know if not.

Yes, you are right. Fantastic finding without me even providing the cmake 
command.
But it was true that for the failing stage we had disabled tests using 
`-DLLVM_INCLUDE_TESTS=OFF` which result in CLANG_INCLUDE_TESTS being OFF as 
well. So your fix to respect the CLANG_INCLUDE_TESTS setting does indeed solve 
the problem. Thanks alot for the help!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121233

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

[PATCH] D121122: Set FLT_EVAL_METHOD to -1 when fast-math is enabled.

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

Hello. We've got some problem in our downstream tests after this patch and I'm 
trying to figure out how things are supposed to work. Maybe someone being part 
of this review knows.

Problem is that we have some libc verification suites that include test cases 
using `float_t`. But in math.h from for example newlib the type float_t isn't 
defined if FLT_EVAL_METHOD is set to -1.
The standard says that float_t is implementation defined when FLT_EVAL_METHOD 
isn't 0/1/2. But I'm not quite sure how that is supposed to be handled (such as 
where to define float_t if not in math.h).

One question is: If I have a piece of C99 code using float_t, is that code not 
allowed to be compiled using fast-math?

I guess it should be seen as with fast-math the float_t precision is unknown. 
But the type still has to be defined somewhere or else the frontend will 
complain about "unknown type name". But I'm not sure where this is supposed to 
be handled.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121122

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

[PATCH] D121122: Set FLT_EVAL_METHOD to -1 when fast-math is enabled.

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

Looked around a bit.

- Seems like in GNU libc they use an internal `__GLIBC_FLT_EVAL_METHOD` that is 
set to 2 if `__FLT_EVAL_METHOD__` is -1   
(https://sourceware.org/git/?p=glibc.git;a=blob;f=bits/flt-eval-method.h;hb=f60e45ba10f0ca2794318de95720cdbdb6ff20d0).
- In LLVM:s libc I do not see anything like that, instead there is an `#error` 
if `__FLT_EVAL_METHOD__` is -1  
(https://github.com/llvm/llvm-project/blob/main/libc/include/llvm-libc-types/float_t.h).
- Looking at newlib it seem to do nothing for `__FLT_EVAL_METHOD__` equal to 
-1, although it has some comments about "Assume float_t and double_t have been 
defined previously for this configuration (e.g. config.h)"  
(https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=newlib/libc/include/math.h;h=799ac494adc3f134a8f63a06397e833ec89c644a;hb=HEAD#l169)

So maybe I need to patch our newlib version somehow to make sure float_t is 
defined even for -1. But there could be problems with the LLVM libc 
implementation of math.h (which I think uses float_t.h).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121122

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

[PATCH] D99439: Update @llvm.powi to handle different int sizes for the exponent

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:581
RTLIB::POWI_F128,
RTLIB::POWI_PPCF128);
   if (!TLI.getLibcallName(LC)) {

efriedma wrote:
> This is missing a diagnostic for the exponent.  We don't want to silently 
> miscompile if someone uses an exponent that isn't supported by the target.
Not sure exactly what you suggest. Is that a general comment for all places in 
SelectionDAG where we may emit calls to RTLIB::POWI or what makes this 
SoftenFloatRes special?

If we end up using mismatching types in the call, wouldn't that being detected 
as ICE elsewhere? Only reason I made changes to this function in the first 
place was due to the historical assert above regarding the type of the exponent 
in FPOWI. Maybe I should just drop that assert instead? This is the only place 
where that is checked, but I figure that the SoftenFloatRes legalization is 
just one out of many places where FPOWI is legalized and lowered into libcalls 
to RTLIB::POWI.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99439

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

[PATCH] D99439: Update @llvm.powi to handle different int sizes for the exponent

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeFloatTypes.cpp:581
RTLIB::POWI_F128,
RTLIB::POWI_PPCF128);
   if (!TLI.getLibcallName(LC)) {

efriedma wrote:
> bjope wrote:
> > efriedma wrote:
> > > This is missing a diagnostic for the exponent.  We don't want to silently 
> > > miscompile if someone uses an exponent that isn't supported by the target.
> > Not sure exactly what you suggest. Is that a general comment for all places 
> > in SelectionDAG where we may emit calls to RTLIB::POWI or what makes this 
> > SoftenFloatRes special?
> > 
> > If we end up using mismatching types in the call, wouldn't that being 
> > detected as ICE elsewhere? Only reason I made changes to this function in 
> > the first place was due to the historical assert above regarding the type 
> > of the exponent in FPOWI. Maybe I should just drop that assert instead? 
> > This is the only place where that is checked, but I figure that the 
> > SoftenFloatRes legalization is just one out of many places where FPOWI is 
> > legalized and lowered into libcalls to RTLIB::POWI.
> It's a general issue with emitting calls to RTLIB::POWI; the second parameter 
> to the call has to have type "int", to match the definition in 
> libgcc/compiler-rt.  I guess there are a few other places that also emit 
> calls to these functions.
> 
> > If we end up using mismatching types in the call, wouldn't that being 
> > detected as ICE elsewhere?
> 
> In SelectionDAG, function/pointer types don't exist; the callee of a function 
> call is just a integer.  So we'd never detect mismatched types; we'd just 
> silently emit a call using the wrong calling convention.
One interesting thing when trying to add checks verifying that 
`DAG.getLibInfo().getIntSize() == Node->getOperand(1 + 
Offset).getValueType().getSizeInBits())` in LegalizeDAG some RISCV (64-bit) 
test cases fail. Looks like type legalization is promoting the exponent by 
replacing

```
  t5: i64,ch = CopyFromReg t0, Register:i64 %1
t6: i32 = truncate t5
  t7: f32 = fpowi t3, t6

```
by

```
  t5: i64,ch = CopyFromReg t0, Register:i64 %1
t13: i64 = sign_extend_inreg t5, ValueType:ch:i32
  t7: f32 = fpowi t3, t13
```
I kind of suspect that promoting the exponent for FPOWI always would be 
incorrect, if the idea is that the type always should match with sizeof(int).

In this case RISCV would lower the fpowi to a libcall like this

```
  t5: i64,ch = CopyFromReg t0, Register:i64 %1
t13: i64 = sign_extend_inreg t5, ValueType:ch:i32
  t20: ch,glue = CopyToReg t18, Register:i64 $x11, t13, t18:1
  t23: ch,glue = RISCVISD::CALL t20, TargetExternalSymbol:i64'__powisf2' 
[TF=2], Register:i64 $x10, Register:i64 $x11, RegisterMask:Untyped, t20:1
```
using a 64-bit argument for the call, while the callee expects a 32-bit int. 
Depending on the calling conventions for RISCV64 I suppose this might work by 
coincidence, or it is a bad miscompile.

Not sure exactly how to deal with that when considering this patch. I was kind 
of aiming at fixing problems for 16-bit targets. Maybe we need to deal with 
DAGTypeLegalizer::PromoteIntOp_FPOWI first, turning it into a fault situation. 
And to do that one need to handle FPOWI for RISCV in some sort of way to make 
the 32-bit exponent legal first?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99439

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

[PATCH] D102723: [HIP] Tighten checks in hip-include-path.hip test case

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope created this revision.
bjope added reviewers: yaxunl, tra, zhuhan0.
bjope requested review of this revision.
Herald added a project: clang.

The checks (both positive and negative checks) in the test case
hip-include-path.hip could mistakenly end up matching the string
"clang" from the InstalledDir in case the build dir for example
was named "/home/username/build-clang/". Intention with this
patch is to tighten up the checks a bit to filter our the
part of the paths that match with InstalledDir when doing the
checks, as well as matching "/lib/clang/" rather than
just "clang/".

Problem was found when building with

  -DCLANG_DEFAULT_RTLIB=compiler-rt
  -DCLANG_DEFAULT_CXX_STDLIB=libc++

and having "clang/" in the path to the build dir.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D102723

Files:
  clang/test/Driver/hip-include-path.hip


Index: clang/test/Driver/hip-include-path.hip
===
--- clang/test/Driver/hip-include-path.hip
+++ clang/test/Driver/hip-include-path.hip
@@ -16,36 +16,40 @@
 // RUN:   -std=c++11 --rocm-path=%S/Inputs/rocm -nogpuinc -nogpulib %s 2>&1 \
 // RUN:   | FileCheck -check-prefixes=COMMON,CLANG,NOHIP %s
 
+// COMMON: InstalledDir: [[ROOT:[^"]*]]/bin
+
 // COMMON-LABEL: "{{[^"]*}}clang{{[^"]*}}" "-cc1"
-// CLANG-SAME: "-internal-isystem" "{{.*}}clang/{{.*}}/include/cuda_wrappers"
-// NOCLANG-NOT: "{{.*}}clang/{{.*}}/include/cuda_wrappers"
+// CLANG-SAME: "-internal-isystem" 
"[[ROOT]]/lib/clang/{{.*}}/include/cuda_wrappers"
+// NOCLANG-NOT: "[[ROOT]]/lib/clang/{{.*}}/include/cuda_wrappers"
 // HIP-SAME: "-internal-isystem" "{{.*}}Inputs/rocm/include"
 // NOHIP-NOT: "{{.*}}Inputs/rocm/include"
 // HIP-SAME: "-include" "__clang_hip_runtime_wrapper.h"
 // NOHIP-NOT: "-include" "__clang_hip_runtime_wrapper.h"
 // skip check of standard C++ include path
-// CLANG-SAME: "-internal-isystem" "{{.*}}clang/{{.*}}/include"
-// NOCLANG-NOT: "{{.*}}clang/{{.*}}/include"
+// CLANG-SAME: "-internal-isystem" "[[ROOT]]/lib/clang/{{.*}}/include"
+// NOCLANG-NOT: "[[ROOT]]/lib/clang/{{.*}}/include"
 
 // COMMON-LABEL: "{{[^"]*}}clang{{[^"]*}}" "-cc1"
-// CLANG-SAME: "-internal-isystem" "{{.*}}clang/{{.*}}/include/cuda_wrappers"
-// NOCLANG-NOT: "{{.*}}clang/{{.*}}/include/cuda_wrappers"
+// CLANG-SAME: "-internal-isystem" 
"[[ROOT]]/lib/clang/{{.*}}/include/cuda_wrappers"
+// NOCLANG-NOT: "[[ROOT]]/lib/clang/{{.*}}/include/cuda_wrappers"
 // HIP-SAME: "-internal-isystem" "{{.*}}Inputs/rocm/include"
 // NOHIP-NOT: "{{.*}}Inputs/rocm/include"
 // HIP-SAME: "-include" "__clang_hip_runtime_wrapper.h"
 // NOHIP-NOT: "-include" "__clang_hip_runtime_wrapper.h"
 // skip check of standard C++ include path
-// CLANG-SAME: "-internal-isystem" "{{.*}}clang/{{.*}}/include"
-// NOCLANG-NOT: "{{.*}}clang/{{.*}}/include"
+// CLANG-SAME: "-internal-isystem" "[[ROOT]]/lib/clang/{{.*}}/include"
+// NOCLANG-NOT: "[[ROOT]]/lib/clang/{{.*}}/include"
 
 // RUN: %clang -c -### -target x86_64-unknown-linux-gnu --cuda-gpu-arch=gfx900 
\
 // RUN:   -std=c++11 --rocm-path=%S/Inputs/rocm -nogpulib %s 2>&1 \
 // RUN:   --hip-version=3.5 | FileCheck -check-prefixes=ROCM35 %s
 
+// ROCM35: InstalledDir: [[ROOT:[^"]*]]/bin
+
 // ROCM35-LABEL: "{{[^"]*}}clang{{[^"]*}}" "-cc1"
-// ROCM35-NOT: "{{.*}}clang/{{.*}}/include/cuda_wrappers"
-// ROCM35-SAME: "-internal-isystem" "{{[^"]*}}clang/{{[^"]*}}"
+// ROCM35-NOT: "[[ROOT]]/lib/clang/{{.*}}/include/cuda_wrappers"
+// ROCM35-SAME: "-internal-isystem" "[[ROOT]]/lib/clang/{{[^"]*}}"
 // ROCM35-SAME: "-internal-isystem" "{{[^"]*}}Inputs/rocm/include"
 // ROCM35-NOT: "-include" "__clang_hip_runtime_wrapper.h"
 // skip check of standard C++ include path
-// ROCM35-SAME: "-internal-isystem" "{{[^"]*}}clang/{{[^"]*}}/include"
+// ROCM35-SAME: "-internal-isystem" "[[ROOT]]/lib/clang/{{.*}}/include"


Index: clang/test/Driver/hip-include-path.hip
===
--- clang/test/Driver/hip-include-path.hip
+++ clang/test/Driver/hip-include-path.hip
@@ -16,36 +16,40 @@
 // RUN:   -std=c++11 --rocm-path=%S/Inputs/rocm -nogpuinc -nogpulib %s 2>&1 \
 // RUN:   | FileCheck -check-prefixes=COMMON,CLANG,NOHIP %s
 
+// COMMON: InstalledDir: [[ROOT:[^"]*]]/bin
+
 // COMMON-LABEL: "{{[^"]*}}clang{{[^"]*}}" "-cc1"
-// CLANG-SAME: "-internal-isystem" "{{.*}}clang/{{.*}}/include/cuda_wrappers"
-// NOCLANG-NOT: "{{.*}}clang/{{.*}}/include/cuda_wrappers"
+// CLANG-SAME: "-internal-isystem" "[[ROOT]]/lib/clang/{{.*}}/include/cuda_wrappers"
+// NOCLANG-NOT: "[[ROOT]]/lib/clang/{{.*}}/include/cuda_wrappers"
 // HIP-SAME: "-internal-isystem" "{{.*}}Inputs/rocm/include"
 // NOHIP-NOT: "{{.*}}Inputs/rocm/include"
 // HIP-SAME: "-include" "__clang_hip_runtime_wrapper.h"
 // NOHIP-NOT: "-include" "__clang_hip_runtime_wrapper.h"
 // skip check of standard C++ include path
-// CLANG-SAME: "-internal-isystem" "{{.*}}clang/{{.*}}/include"
-// NOCLANG-NOT: "{{.*}}clang/{{.*}}/include"

[PATCH] D102723: [HIP] Tighten checks in hip-include-path.hip test case

[Bjorn Pettersson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG722c39fef5ab: [HIP] Tighten checks in hip-include-path.hip 
test case (authored by bjope).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102723

Files:
  clang/test/Driver/hip-include-path.hip


Index: clang/test/Driver/hip-include-path.hip
===
--- clang/test/Driver/hip-include-path.hip
+++ clang/test/Driver/hip-include-path.hip
@@ -16,36 +16,40 @@
 // RUN:   -std=c++11 --rocm-path=%S/Inputs/rocm -nogpuinc -nogpulib %s 2>&1 \
 // RUN:   | FileCheck -check-prefixes=COMMON,CLANG,NOHIP %s
 
+// COMMON: InstalledDir: [[ROOT:[^"]*]]/bin
+
 // COMMON-LABEL: "{{[^"]*}}clang{{[^"]*}}" "-cc1"
-// CLANG-SAME: "-internal-isystem" "{{.*}}clang/{{.*}}/include/cuda_wrappers"
-// NOCLANG-NOT: "{{.*}}clang/{{.*}}/include/cuda_wrappers"
+// CLANG-SAME: "-internal-isystem" 
"[[ROOT]]/lib/clang/{{.*}}/include/cuda_wrappers"
+// NOCLANG-NOT: "[[ROOT]]/lib/clang/{{.*}}/include/cuda_wrappers"
 // HIP-SAME: "-internal-isystem" "{{.*}}Inputs/rocm/include"
 // NOHIP-NOT: "{{.*}}Inputs/rocm/include"
 // HIP-SAME: "-include" "__clang_hip_runtime_wrapper.h"
 // NOHIP-NOT: "-include" "__clang_hip_runtime_wrapper.h"
 // skip check of standard C++ include path
-// CLANG-SAME: "-internal-isystem" "{{.*}}clang/{{.*}}/include"
-// NOCLANG-NOT: "{{.*}}clang/{{.*}}/include"
+// CLANG-SAME: "-internal-isystem" "[[ROOT]]/lib/clang/{{.*}}/include"
+// NOCLANG-NOT: "[[ROOT]]/lib/clang/{{.*}}/include"
 
 // COMMON-LABEL: "{{[^"]*}}clang{{[^"]*}}" "-cc1"
-// CLANG-SAME: "-internal-isystem" "{{.*}}clang/{{.*}}/include/cuda_wrappers"
-// NOCLANG-NOT: "{{.*}}clang/{{.*}}/include/cuda_wrappers"
+// CLANG-SAME: "-internal-isystem" 
"[[ROOT]]/lib/clang/{{.*}}/include/cuda_wrappers"
+// NOCLANG-NOT: "[[ROOT]]/lib/clang/{{.*}}/include/cuda_wrappers"
 // HIP-SAME: "-internal-isystem" "{{.*}}Inputs/rocm/include"
 // NOHIP-NOT: "{{.*}}Inputs/rocm/include"
 // HIP-SAME: "-include" "__clang_hip_runtime_wrapper.h"
 // NOHIP-NOT: "-include" "__clang_hip_runtime_wrapper.h"
 // skip check of standard C++ include path
-// CLANG-SAME: "-internal-isystem" "{{.*}}clang/{{.*}}/include"
-// NOCLANG-NOT: "{{.*}}clang/{{.*}}/include"
+// CLANG-SAME: "-internal-isystem" "[[ROOT]]/lib/clang/{{.*}}/include"
+// NOCLANG-NOT: "[[ROOT]]/lib/clang/{{.*}}/include"
 
 // RUN: %clang -c -### -target x86_64-unknown-linux-gnu --cuda-gpu-arch=gfx900 
\
 // RUN:   -std=c++11 --rocm-path=%S/Inputs/rocm -nogpulib %s 2>&1 \
 // RUN:   --hip-version=3.5 | FileCheck -check-prefixes=ROCM35 %s
 
+// ROCM35: InstalledDir: [[ROOT:[^"]*]]/bin
+
 // ROCM35-LABEL: "{{[^"]*}}clang{{[^"]*}}" "-cc1"
-// ROCM35-NOT: "{{.*}}clang/{{.*}}/include/cuda_wrappers"
-// ROCM35-SAME: "-internal-isystem" "{{[^"]*}}clang/{{[^"]*}}"
+// ROCM35-NOT: "[[ROOT]]/lib/clang/{{.*}}/include/cuda_wrappers"
+// ROCM35-SAME: "-internal-isystem" "[[ROOT]]/lib/clang/{{[^"]*}}"
 // ROCM35-SAME: "-internal-isystem" "{{[^"]*}}Inputs/rocm/include"
 // ROCM35-NOT: "-include" "__clang_hip_runtime_wrapper.h"
 // skip check of standard C++ include path
-// ROCM35-SAME: "-internal-isystem" "{{[^"]*}}clang/{{[^"]*}}/include"
+// ROCM35-SAME: "-internal-isystem" "[[ROOT]]/lib/clang/{{.*}}/include"


Index: clang/test/Driver/hip-include-path.hip
===
--- clang/test/Driver/hip-include-path.hip
+++ clang/test/Driver/hip-include-path.hip
@@ -16,36 +16,40 @@
 // RUN:   -std=c++11 --rocm-path=%S/Inputs/rocm -nogpuinc -nogpulib %s 2>&1 \
 // RUN:   | FileCheck -check-prefixes=COMMON,CLANG,NOHIP %s
 
+// COMMON: InstalledDir: [[ROOT:[^"]*]]/bin
+
 // COMMON-LABEL: "{{[^"]*}}clang{{[^"]*}}" "-cc1"
-// CLANG-SAME: "-internal-isystem" "{{.*}}clang/{{.*}}/include/cuda_wrappers"
-// NOCLANG-NOT: "{{.*}}clang/{{.*}}/include/cuda_wrappers"
+// CLANG-SAME: "-internal-isystem" "[[ROOT]]/lib/clang/{{.*}}/include/cuda_wrappers"
+// NOCLANG-NOT: "[[ROOT]]/lib/clang/{{.*}}/include/cuda_wrappers"
 // HIP-SAME: "-internal-isystem" "{{.*}}Inputs/rocm/include"
 // NOHIP-NOT: "{{.*}}Inputs/rocm/include"
 // HIP-SAME: "-include" "__clang_hip_runtime_wrapper.h"
 // NOHIP-NOT: "-include" "__clang_hip_runtime_wrapper.h"
 // skip check of standard C++ include path
-// CLANG-SAME: "-internal-isystem" "{{.*}}clang/{{.*}}/include"
-// NOCLANG-NOT: "{{.*}}clang/{{.*}}/include"
+// CLANG-SAME: "-internal-isystem" "[[ROOT]]/lib/clang/{{.*}}/include"
+// NOCLANG-NOT: "[[ROOT]]/lib/clang/{{.*}}/include"
 
 // COMMON-LABEL: "{{[^"]*}}clang{{[^"]*}}" "-cc1"
-// CLANG-SAME: "-internal-isystem" "{{.*}}clang/{{.*}}/include/cuda_wrappers"
-// NOCLANG-NOT: "{{.*}}clang/{{.*}}/include/cuda_wrappers"
+// CLANG-SAME: "-internal-isystem" "[[ROOT]]/lib/clang/{{.*}}/include/cuda_wrappers"
+// NOCLANG-NOT: "[[ROOT]]/lib/clang/{{.*}}/include/cuda_wrappers"
 // HIP-SAME: "-internal-isystem

[PATCH] D99439: Update @llvm.powi to handle different int sizes for the exponent

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope marked 3 inline comments as done.
bjope added inline comments.



Comment at: llvm/test/Transforms/InstCombine/pow_fp_int16.ll:3
 
-; PR42190
+; Test case was copied from pow_fp_int.ll but adjusted for 16-bit int.
+; Assuming that we can't generate test checks for the same reason (PR42740).

xbolva00 wrote:
> Precommit? And we dont need full copy of existings tests - 2-3 tests for 
> 16bit int are anough.
I've pre-commited the tests now. Remove some of the tests (mainly the ones 
related to i64. But I think most of the others are relevant as regression test 
to see that we get what is expected for the different scenarios also with 
16-bit int.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99439

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

[PATCH] D102723: [HIP] Tighten checks in hip-include-path.hip test case

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

In D102723#2789537 , @yaxunl wrote:

> It seems we cannot introduce `ROOT` by line 19 since it is not used in other 
> lines in situations where working directories have sym links.
>
> Instead, we just change `{{.*}}clang/` to `{{.*}}/lib/clang/` in other lines. 
> Hopefully this will still work.

Ok. That probably works.

A bit surprised though. I found checks using InstalledDir in other test cases 
such as clang/test/Friver/stdlibxx-isystem.cpp, so I figured it would be safe 
to use that also in this test case. So I wonder how other tests that captures 
InstalledDir could work for the symlink-bots.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102723

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

[PATCH] D102723: [HIP] Tighten checks in hip-include-path.hip test case

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

I relaxed the checks a bit again here: https://reviews.llvm.org/rGf0e10cc91bc4
But it looks like the workers here (https://lab.llvm.org/staging/#/workers/109) 
are paused so hard to tell if it helped.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102723

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

[PATCH] D103386: [PowerPC] Fix x86 vector intrinsics wrapper compilation under C++

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added subscribers: phosek, bjope.
bjope added inline comments.



Comment at: clang/test/CodeGen/ppc-xmmintrin.c:10
 // RUN:   -fno-discard-value-names -mllvm -disable-llvm-optzns -o - | 
llvm-cxxfilt -n | FileCheck %s --check-prefixes=CHECK,CHECK-LE
+// RUN: %clang -x c++ -fsyntax-only -target powerpc64le-unknown-linux-gnu 
-mcpu=pwr8 -ffreestanding -DNO_WARN_X86_INTRINSICS %s \
+// RUN:   -fno-discard-value-names -mllvm -disable-llvm-optzns

Unfortunately I get some failures with this. Maybe because of an unstandard 
build setup.

We've started to use `-DCLANG_DEFAULT_RTLIB=compiler-rt 
-DCLANG_DEFAULT_CXX_STDLIB=libc++` when building clang. And we also use 
`-DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON`. Not sure if that is a setup that is 
asking for trouble. Anyway, when running this test case we end up with

```
: 'RUN: at line 10';   /workspace/llvm/build/bin/clang -x c++ -fsyntax-only 
-target powerpc64le-unknown-linux-gnu -mcpu=pwr8 -ffreestanding 
-DNO_WARN_X86_INTRINSICS /workspace/clang/test/CodeGen/ppc-xmmintrin.c
-fno-discard-value-names -mllvm -disable-llvm-optzns
--
Exit Code: 1

Command Output (stderr):
--
In file included from /workspace/clang/test/CodeGen/ppc-xmmintrin.c:13:
In file included from 
/workspace/llvm/build/lib/clang/13.0.0/include/ppc_wrappers/xmmintrin.h:42:
In file included from 
/workspace/llvm/build/lib/clang/13.0.0/include/altivec.h:44:
In file included from /workspace/llvm/build/bin/../include/c++/v1/stddef.h:39:
/workspace/llvm/build/bin/../include/c++/v1/__config:13:10: fatal error: 
'__config_site' file not found
#include <__config_site>
 ^~~
1 error generated.
```

Not sure really how to solve that.

Maybe we should stop building like that?

Or there is a bug somewhere (such as that we only get the __config_site headers 
in target specific dirs for targets that we actually build runtimes for, maybe 
something that was missing in https://reviews.llvm.org/D97572)?
(Maybe @phosek  could comment on that?)

Or this test case is missing some options to make it a bit more independent on 
the runtimes build setup?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103386

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

[PATCH] D101192: Add support for #elifdef and #elifndef

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang/lib/Lex/PPDirectives.cpp:594
+if (CondInfo.FoundElse)
+  Diag(Tok, diag::pp_err_elif_after_else) << PED_Elif;
 

Hi @aaron.ballman 

This change is missing from 
https://reviews.llvm.org/rG8edd3464afbff65d7d5945b3a8b20009d6ff5deb , while you 
instead got it when doing the pp_err_else_after_else diagnostic a couple of 
lines above this.

It causes asserts (in DIagnostic::getArgKind) for me in test cases verifying 
the "elif after else" scenario using a test case basically doing
```
#if 1
#else
#elif
#endif
```
So maybe such a test case should be added as well?

But it seems like the patch you committed doesn't match with the reviewed 
patch. So maybe you can look into this and fix it?
(let me know if you need some additional help)


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

https://reviews.llvm.org/D101192

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

[PATCH] D101192: Add support for #elifdef and #elifndef

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang/lib/Lex/PPDirectives.cpp:594
+if (CondInfo.FoundElse)
+  Diag(Tok, diag::pp_err_elif_after_else) << PED_Elif;
 

aaron.ballman wrote:
> bjope wrote:
> > Hi @aaron.ballman 
> > 
> > This change is missing from 
> > https://reviews.llvm.org/rG8edd3464afbff65d7d5945b3a8b20009d6ff5deb , while 
> > you instead got it when doing the pp_err_else_after_else diagnostic a 
> > couple of lines above this.
> > 
> > It causes asserts (in DIagnostic::getArgKind) for me in test cases 
> > verifying the "elif after else" scenario using a test case basically doing
> > ```
> > #if 1
> > #else
> > #elif
> > #endif
> > ```
> > So maybe such a test case should be added as well?
> > 
> > But it seems like the patch you committed doesn't match with the reviewed 
> > patch. So maybe you can look into this and fix it?
> > (let me know if you need some additional help)
> Great catch and thank you for the test case! It seems that when I applied the 
> patch, it misapplied, but not in a way that caused a test failure. I have no 
> idea how that happened, but I assume I screwed up something with git 
> somewhere along the way and caused the failure.
> 
> I've pushed baa2b8d08502acfa91a8dfd699d25f7b4e25edbb to fix it. Thanks!
Big thanks for the quick fix! While I managed to track down this problem I 
didn't have time to properly fix it right away. Now I'll have one thing less to 
worry about tomorrow :-)


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

https://reviews.llvm.org/D101192

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

[PATCH] D103386: [PowerPC] Fix x86 vector intrinsics wrapper compilation under C++

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang/test/CodeGen/ppc-xmmintrin.c:10
 // RUN:   -fno-discard-value-names -mllvm -disable-llvm-optzns -o - | 
llvm-cxxfilt -n | FileCheck %s --check-prefixes=CHECK,CHECK-LE
+// RUN: %clang -x c++ -fsyntax-only -target powerpc64le-unknown-linux-gnu 
-mcpu=pwr8 -ffreestanding -DNO_WARN_X86_INTRINSICS %s \
+// RUN:   -fno-discard-value-names -mllvm -disable-llvm-optzns

qiucf wrote:
> bjope wrote:
> > Unfortunately I get some failures with this. Maybe because of an unstandard 
> > build setup.
> > 
> > We've started to use `-DCLANG_DEFAULT_RTLIB=compiler-rt 
> > -DCLANG_DEFAULT_CXX_STDLIB=libc++` when building clang. And we also use 
> > `-DLLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON`. Not sure if that is a setup that 
> > is asking for trouble. Anyway, when running this test case we end up with
> > 
> > ```
> > : 'RUN: at line 10';   /workspace/llvm/build/bin/clang -x c++ -fsyntax-only 
> > -target powerpc64le-unknown-linux-gnu -mcpu=pwr8 -ffreestanding 
> > -DNO_WARN_X86_INTRINSICS /workspace/clang/test/CodeGen/ppc-xmmintrin.c
> > -fno-discard-value-names -mllvm -disable-llvm-optzns
> > --
> > Exit Code: 1
> > 
> > Command Output (stderr):
> > --
> > In file included from /workspace/clang/test/CodeGen/ppc-xmmintrin.c:13:
> > In file included from 
> > /workspace/llvm/build/lib/clang/13.0.0/include/ppc_wrappers/xmmintrin.h:42:
> > In file included from 
> > /workspace/llvm/build/lib/clang/13.0.0/include/altivec.h:44:
> > In file included from 
> > /workspace/llvm/build/bin/../include/c++/v1/stddef.h:39:
> > /workspace/llvm/build/bin/../include/c++/v1/__config:13:10: fatal error: 
> > '__config_site' file not found
> > #include <__config_site>
> >  ^~~
> > 1 error generated.
> > ```
> > 
> > Not sure really how to solve that.
> > 
> > Maybe we should stop building like that?
> > 
> > Or there is a bug somewhere (such as that we only get the __config_site 
> > headers in target specific dirs for targets that we actually build runtimes 
> > for, maybe something that was missing in https://reviews.llvm.org/D97572)?
> > (Maybe @phosek  could comment on that?)
> > 
> > Or this test case is missing some options to make it a bit more independent 
> > on the runtimes build setup?
> > 
> The tests relies on some system stuff. Is the failure related to this change? 
> (or exposed by `-x c++` option?)
Well, i guess it was exposed by `-x c++` (in combination with D975729).

I don't really understand how things are supposed to work given the pre-target 
specific `__config_site`.

Since I only build libcxx for `x86_64-unknown-linux-gnu`, I only get a 
`__config_site` file for that specific triple inside 
`bin/../include/x86_64-unknown-linux-gnu/c++/v1/__config_site` in the build 
result. But a test case like this one, using a different triple, ends up 
including `bin/../include/c++/v1/stddef.h`, that wants wants to include 
`<__config_site>`, but it won't find any such include for the 
powerpc64le-unknown-linux-gnu triple.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103386

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

[PATCH] D94977: [CodeGen] Use getCharWidth() more consistently in CGRecordLowering. NFC

[Bjorn Pettersson via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG72f863fd37c3: [CodeGen] Use getCharWidth() more consistently 
in CGRecordLowering. NFC (authored by bjope).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94977

Files:
  clang/lib/CodeGen/CGRecordLayoutBuilder.cpp


Index: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
===
--- clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
+++ clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
@@ -127,15 +127,20 @@
 
   /// Wraps llvm::Type::getIntNTy with some implicit arguments.
   llvm::Type *getIntNType(uint64_t NumBits) {
+unsigned AlignedBits = llvm::alignTo(NumBits, Context.getCharWidth());
+return llvm::Type::getIntNTy(Types.getLLVMContext(), AlignedBits);
+  }
+  /// Get the LLVM type sized as one character unit.
+  llvm::Type *getCharType() {
 return llvm::Type::getIntNTy(Types.getLLVMContext(),
- (unsigned)llvm::alignTo(NumBits, 8));
+ Context.getCharWidth());
   }
-  /// Gets an llvm type of size NumBytes and alignment 1.
-  llvm::Type *getByteArrayType(CharUnits NumBytes) {
-assert(!NumBytes.isZero() && "Empty byte arrays aren't allowed.");
-llvm::Type *Type = llvm::Type::getInt8Ty(Types.getLLVMContext());
-return NumBytes == CharUnits::One() ? Type :
-(llvm::Type *)llvm::ArrayType::get(Type, NumBytes.getQuantity());
+  /// Gets an llvm type of size NumChars and alignment 1.
+  llvm::Type *getByteArrayType(CharUnits NumChars) {
+assert(!NumChars.isZero() && "Empty byte arrays aren't allowed.");
+llvm::Type *Type = getCharType();
+return NumChars == CharUnits::One() ? Type :
+(llvm::Type *)llvm::ArrayType::get(Type, NumChars.getQuantity());
   }
   /// Gets the storage type for a field decl and handles storage
   /// for itanium bitfields that are smaller than their declared type.


Index: clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
===
--- clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
+++ clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
@@ -127,15 +127,20 @@
 
   /// Wraps llvm::Type::getIntNTy with some implicit arguments.
   llvm::Type *getIntNType(uint64_t NumBits) {
+unsigned AlignedBits = llvm::alignTo(NumBits, Context.getCharWidth());
+return llvm::Type::getIntNTy(Types.getLLVMContext(), AlignedBits);
+  }
+  /// Get the LLVM type sized as one character unit.
+  llvm::Type *getCharType() {
 return llvm::Type::getIntNTy(Types.getLLVMContext(),
- (unsigned)llvm::alignTo(NumBits, 8));
+ Context.getCharWidth());
   }
-  /// Gets an llvm type of size NumBytes and alignment 1.
-  llvm::Type *getByteArrayType(CharUnits NumBytes) {
-assert(!NumBytes.isZero() && "Empty byte arrays aren't allowed.");
-llvm::Type *Type = llvm::Type::getInt8Ty(Types.getLLVMContext());
-return NumBytes == CharUnits::One() ? Type :
-(llvm::Type *)llvm::ArrayType::get(Type, NumBytes.getQuantity());
+  /// Gets an llvm type of size NumChars and alignment 1.
+  llvm::Type *getByteArrayType(CharUnits NumChars) {
+assert(!NumChars.isZero() && "Empty byte arrays aren't allowed.");
+llvm::Type *Type = getCharType();
+return NumChars == CharUnits::One() ? Type :
+(llvm::Type *)llvm::ArrayType::get(Type, NumChars.getQuantity());
   }
   /// Gets the storage type for a field decl and handles storage
   /// for itanium bitfields that are smaller than their declared type.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94979: [CGExpr] Use getCharWidth() more consistently in CCGExprConstant. NFC

[Bjorn Pettersson via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGea2cfda386f1: [CGExpr] Use getCharWidth() more consistently 
in CCGExprConstant. NFC (authored by bjope).

Changed prior to commit:
  https://reviews.llvm.org/D94979?vs=318135&id=318607#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94979

Files:
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenTypeCache.h


Index: clang/lib/CodeGen/CodeGenTypeCache.h
===
--- clang/lib/CodeGen/CodeGenTypeCache.h
+++ clang/lib/CodeGen/CodeGenTypeCache.h
@@ -41,6 +41,9 @@
   /// int
   llvm::IntegerType *IntTy;
 
+  /// char
+  llvm::IntegerType *CharTy;
+
   /// intptr_t, size_t, and ptrdiff_t, which we assume are the same size.
   union {
 llvm::IntegerType *IntPtrTy;
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -123,6 +123,8 @@
 
C.toCharUnitsFromBits(C.getTargetInfo().getMaxPointerWidth()).getQuantity();
   IntAlignInBytes =
 C.toCharUnitsFromBits(C.getTargetInfo().getIntAlign()).getQuantity();
+  CharTy =
+llvm::IntegerType::get(LLVMContext, C.getTargetInfo().getCharWidth());
   IntTy = llvm::IntegerType::get(LLVMContext, C.getTargetInfo().getIntWidth());
   IntPtrTy = llvm::IntegerType::get(LLVMContext,
 C.getTargetInfo().getMaxPointerWidth());
Index: clang/lib/CodeGen/CGExprConstant.cpp
===
--- clang/lib/CodeGen/CGExprConstant.cpp
+++ clang/lib/CodeGen/CGExprConstant.cpp
@@ -58,14 +58,14 @@
   }
 
   llvm::Constant *getPadding(CharUnits PadSize) const {
-llvm::Type *Ty = CGM.Int8Ty;
+llvm::Type *Ty = CGM.CharTy;
 if (PadSize > CharUnits::One())
   Ty = llvm::ArrayType::get(Ty, PadSize.getQuantity());
 return llvm::UndefValue::get(Ty);
   }
 
   llvm::Constant *getZeroes(CharUnits ZeroSize) const {
-llvm::Type *Ty = llvm::ArrayType::get(CGM.Int8Ty, ZeroSize.getQuantity());
+llvm::Type *Ty = llvm::ArrayType::get(CGM.CharTy, ZeroSize.getQuantity());
 return llvm::ConstantAggregateZero::get(Ty);
   }
 };
@@ -1069,7 +1069,7 @@
 
   assert(CurSize <= TotalSize && "Union size mismatch!");
   if (unsigned NumPadBytes = TotalSize - CurSize) {
-llvm::Type *Ty = CGM.Int8Ty;
+llvm::Type *Ty = CGM.CharTy;
 if (NumPadBytes > 1)
   Ty = llvm::ArrayType::get(Ty, NumPadBytes);
 


Index: clang/lib/CodeGen/CodeGenTypeCache.h
===
--- clang/lib/CodeGen/CodeGenTypeCache.h
+++ clang/lib/CodeGen/CodeGenTypeCache.h
@@ -41,6 +41,9 @@
   /// int
   llvm::IntegerType *IntTy;
 
+  /// char
+  llvm::IntegerType *CharTy;
+
   /// intptr_t, size_t, and ptrdiff_t, which we assume are the same size.
   union {
 llvm::IntegerType *IntPtrTy;
Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -123,6 +123,8 @@
 C.toCharUnitsFromBits(C.getTargetInfo().getMaxPointerWidth()).getQuantity();
   IntAlignInBytes =
 C.toCharUnitsFromBits(C.getTargetInfo().getIntAlign()).getQuantity();
+  CharTy =
+llvm::IntegerType::get(LLVMContext, C.getTargetInfo().getCharWidth());
   IntTy = llvm::IntegerType::get(LLVMContext, C.getTargetInfo().getIntWidth());
   IntPtrTy = llvm::IntegerType::get(LLVMContext,
 C.getTargetInfo().getMaxPointerWidth());
Index: clang/lib/CodeGen/CGExprConstant.cpp
===
--- clang/lib/CodeGen/CGExprConstant.cpp
+++ clang/lib/CodeGen/CGExprConstant.cpp
@@ -58,14 +58,14 @@
   }
 
   llvm::Constant *getPadding(CharUnits PadSize) const {
-llvm::Type *Ty = CGM.Int8Ty;
+llvm::Type *Ty = CGM.CharTy;
 if (PadSize > CharUnits::One())
   Ty = llvm::ArrayType::get(Ty, PadSize.getQuantity());
 return llvm::UndefValue::get(Ty);
   }
 
   llvm::Constant *getZeroes(CharUnits ZeroSize) const {
-llvm::Type *Ty = llvm::ArrayType::get(CGM.Int8Ty, ZeroSize.getQuantity());
+llvm::Type *Ty = llvm::ArrayType::get(CGM.CharTy, ZeroSize.getQuantity());
 return llvm::ConstantAggregateZero::get(Ty);
   }
 };
@@ -1069,7 +1069,7 @@
 
   assert(CurSize <= TotalSize && "Union size mismatch!");
   if (unsigned NumPadBytes = TotalSize - CurSize) {
-llvm::Type *Ty = CGM.Int8Ty;
+llvm::Type *Ty = CGM.CharTy;
 if (NumPadBytes > 1)
   Ty = llvm::ArrayType::get(Ty, NumPadBytes);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
htt

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

Kind of the same take on this as @thopre .

I wrote https://bugs.llvm.org/show_bug.cgi?id=48821 , and proposed 
https://reviews.llvm.org/D95789 , when I noticed that this pass caused troubles 
for two reasons:

1. to avoid that others would stumble upon the same problem (getting unexpected 
regressions due to switching pass manager)
2. not that important really, but getting rid of downstream diffs is always 
nice (at least when the problem is likely to cause problems for other targets 
as well)

For the downstream target I've hacked around this a long time ago.

I do think it was a bit sneaky to get this pass by default in the pipeline as 
part of the PM switch. It might have caused lots of trouble for the new-PM 
switch in the past, and still for some downstream contributors.
So in that sense I'm all in favor. Although, a LGTM from me is a bit weak as 
I'm already using a workaround to skip this pass downstream.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104099

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

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: llvm/lib/Passes/PassBuilder.cpp:1449
-  // inserting redundancies into the program. This even includes SimplifyCFG.
-  OptimizePM.addPass(SpeculateAroundPHIsPass());
-

lebedev.ri wrote:
> nikic wrote:
> > As it has been in-tree for a while, I think it would be good to add a 
> > cl::opt option for people who have come to rely on it being present in some 
> > way.
> I'm not convinced that the pass can be returned into it's current position in 
> pipeline,
> that is why the diff is what it is now.
> 
Yes, there is a contradiction between the code comment and the position in the 
pipeline. I did question that here: 
https://bugs.llvm.org/show_bug.cgi?id=48821#c2



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104099

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

[PATCH] D99439: Update @llvm.powi to handle different int sizes for the exponent

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

ping!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99439

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

[PATCH] D99439: Update @llvm.powi to handle different int sizes for the exponent

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

In D99439#2823338 , @efriedma wrote:

> LGTM

Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99439

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

[PATCH] D105007: [NewPM] Update some sanitizer pass names in the PassRegistry

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope updated this revision to Diff 367263.
bjope added a comment.
Herald added subscribers: cfe-commits, ormris.
Herald added a project: clang.

Updated to use _PASS_WITH_PARAMS and option parsing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105007

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/include/llvm/Transforms/Instrumentation/AddressSanitizer.h
  llvm/include/llvm/Transforms/Instrumentation/HWAddressSanitizer.h
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
  llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
  llvm/tools/opt/NewPMDriver.cpp

Index: llvm/tools/opt/NewPMDriver.cpp
===
--- llvm/tools/opt/NewPMDriver.cpp
+++ llvm/tools/opt/NewPMDriver.cpp
@@ -336,18 +336,19 @@
   PB.registerPipelineParsingCallback(
   [](StringRef Name, ModulePassManager &MPM,
  ArrayRef) {
+AddressSanitizerOptions Opts;
 if (Name == "asan-pipeline") {
   MPM.addPass(
   RequireAnalysisPass());
   MPM.addPass(
-  createModuleToFunctionPassAdaptor(AddressSanitizerPass()));
+  createModuleToFunctionPassAdaptor(AddressSanitizerPass(Opts)));
   MPM.addPass(ModuleAddressSanitizerPass());
   return true;
 } else if (Name == "asan-function-pipeline") {
   MPM.addPass(
   RequireAnalysisPass());
   MPM.addPass(
-  createModuleToFunctionPassAdaptor(AddressSanitizerPass()));
+  createModuleToFunctionPassAdaptor(AddressSanitizerPass(Opts)));
   return true;
 }
 return false;
Index: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
===
--- llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
+++ llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
@@ -469,19 +469,14 @@
   DisableOptimization);
 }
 
-HWAddressSanitizerPass::HWAddressSanitizerPass(bool CompileKernel, bool Recover,
-   bool DisableOptimization)
-: CompileKernel(CompileKernel), Recover(Recover),
-  DisableOptimization(DisableOptimization) {}
-
 PreservedAnalyses HWAddressSanitizerPass::run(Module &M,
   ModuleAnalysisManager &MAM) {
   const StackSafetyGlobalInfo *SSI = nullptr;
   auto TargetTriple = llvm::Triple(M.getTargetTriple());
-  if (shouldUseStackSafetyAnalysis(TargetTriple, DisableOptimization))
+  if (shouldUseStackSafetyAnalysis(TargetTriple, Options.DisableOptimization))
 SSI = &MAM.getResult(M);
 
-  HWAddressSanitizer HWASan(M, CompileKernel, Recover, SSI);
+  HWAddressSanitizer HWASan(M, Options.CompileKernel, Options.Recover, SSI);
   bool Modified = false;
   auto &FAM = MAM.getResult(M).getManager();
   for (Function &F : M) {
Index: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
===
--- llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
+++ llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
@@ -1212,20 +1212,14 @@
   return GlobalsMetadata(M);
 }
 
-AddressSanitizerPass::AddressSanitizerPass(
-bool CompileKernel, bool Recover, bool UseAfterScope,
-AsanDetectStackUseAfterReturnMode UseAfterReturn)
-: CompileKernel(CompileKernel), Recover(Recover),
-  UseAfterScope(UseAfterScope), UseAfterReturn(UseAfterReturn) {}
-
 PreservedAnalyses AddressSanitizerPass::run(Function &F,
 AnalysisManager &AM) {
   auto &MAMProxy = AM.getResult(F);
   Module &M = *F.getParent();
   if (auto *R = MAMProxy.getCachedResult(M)) {
 const TargetLibraryInfo *TLI = &AM.getResult(F);
-AddressSanitizer Sanitizer(M, R, CompileKernel, Recover, UseAfterScope,
-   UseAfterReturn);
+AddressSanitizer Sanitizer(M, R, Options.CompileKernel, Options.Recover,
+   Options.UseAfterScope, Options.UseAfterReturn);
 if (Sanitizer.instrumentFunction(F, TLI))
   return PreservedAnalyses::none();
 return PreservedAnalyses::all();
Index: llvm/lib/Passes/PassRegistry.def
===
--- llvm/lib/Passes/PassRegistry.def
+++ llvm/lib/Passes/PassRegistry.def
@@ -60,8 +60,6 @@
 MODULE_PASS("globalopt", GlobalOptPass())
 MODULE_PASS("globalsplit", GlobalSplitPass())
 MODULE_PASS("hotcoldsplit", HotColdSplittingPass())
-MODULE_PASS("hwasan", HWAddressSanitizerPass(false, false))
-MODULE_PASS("khwasan", HWAddressSanitizerPass(true, true))
 MODULE_PASS("inferattrs", InferFunctionAttrsPass())
 MODULE_PASS("inliner-wrapper", ModuleInlinerWrapperPass())
 MODULE_PASS("inliner-wrapper-no-m

[PATCH] D105007: [NewPM] Make some sanitizer passes parameterized in the PassRegistry

[Bjorn Pettersson via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG36d51386195e: [NewPM] Make some sanitizer passes 
parameterized in the PassRegistry (authored by bjope).

Changed prior to commit:
  https://reviews.llvm.org/D105007?vs=367263&id=367445#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105007

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  llvm/include/llvm/Transforms/Instrumentation/AddressSanitizer.h
  llvm/include/llvm/Transforms/Instrumentation/HWAddressSanitizer.h
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Passes/PassRegistry.def
  llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
  llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
  llvm/tools/opt/NewPMDriver.cpp

Index: llvm/tools/opt/NewPMDriver.cpp
===
--- llvm/tools/opt/NewPMDriver.cpp
+++ llvm/tools/opt/NewPMDriver.cpp
@@ -339,18 +339,19 @@
   PB.registerPipelineParsingCallback(
   [](StringRef Name, ModulePassManager &MPM,
  ArrayRef) {
+AddressSanitizerOptions Opts;
 if (Name == "asan-pipeline") {
   MPM.addPass(
   RequireAnalysisPass());
   MPM.addPass(
-  createModuleToFunctionPassAdaptor(AddressSanitizerPass()));
+  createModuleToFunctionPassAdaptor(AddressSanitizerPass(Opts)));
   MPM.addPass(ModuleAddressSanitizerPass());
   return true;
 } else if (Name == "asan-function-pipeline") {
   MPM.addPass(
   RequireAnalysisPass());
   MPM.addPass(
-  createModuleToFunctionPassAdaptor(AddressSanitizerPass()));
+  createModuleToFunctionPassAdaptor(AddressSanitizerPass(Opts)));
   return true;
 }
 return false;
Index: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
===
--- llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
+++ llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp
@@ -471,19 +471,14 @@
   DisableOptimization);
 }
 
-HWAddressSanitizerPass::HWAddressSanitizerPass(bool CompileKernel, bool Recover,
-   bool DisableOptimization)
-: CompileKernel(CompileKernel), Recover(Recover),
-  DisableOptimization(DisableOptimization) {}
-
 PreservedAnalyses HWAddressSanitizerPass::run(Module &M,
   ModuleAnalysisManager &MAM) {
   const StackSafetyGlobalInfo *SSI = nullptr;
   auto TargetTriple = llvm::Triple(M.getTargetTriple());
-  if (shouldUseStackSafetyAnalysis(TargetTriple, DisableOptimization))
+  if (shouldUseStackSafetyAnalysis(TargetTriple, Options.DisableOptimization))
 SSI = &MAM.getResult(M);
 
-  HWAddressSanitizer HWASan(M, CompileKernel, Recover, SSI);
+  HWAddressSanitizer HWASan(M, Options.CompileKernel, Options.Recover, SSI);
   bool Modified = false;
   auto &FAM = MAM.getResult(M).getManager();
   for (Function &F : M) {
Index: llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
===
--- llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
+++ llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
@@ -1212,20 +1212,14 @@
   return GlobalsMetadata(M);
 }
 
-AddressSanitizerPass::AddressSanitizerPass(
-bool CompileKernel, bool Recover, bool UseAfterScope,
-AsanDetectStackUseAfterReturnMode UseAfterReturn)
-: CompileKernel(CompileKernel), Recover(Recover),
-  UseAfterScope(UseAfterScope), UseAfterReturn(UseAfterReturn) {}
-
 PreservedAnalyses AddressSanitizerPass::run(Function &F,
 AnalysisManager &AM) {
   auto &MAMProxy = AM.getResult(F);
   Module &M = *F.getParent();
   if (auto *R = MAMProxy.getCachedResult(M)) {
 const TargetLibraryInfo *TLI = &AM.getResult(F);
-AddressSanitizer Sanitizer(M, R, CompileKernel, Recover, UseAfterScope,
-   UseAfterReturn);
+AddressSanitizer Sanitizer(M, R, Options.CompileKernel, Options.Recover,
+   Options.UseAfterScope, Options.UseAfterReturn);
 if (Sanitizer.instrumentFunction(F, TLI))
   return PreservedAnalyses::none();
 return PreservedAnalyses::all();
Index: llvm/lib/Passes/PassRegistry.def
===
--- llvm/lib/Passes/PassRegistry.def
+++ llvm/lib/Passes/PassRegistry.def
@@ -60,8 +60,6 @@
 MODULE_PASS("globalopt", GlobalOptPass())
 MODULE_PASS("globalsplit", GlobalSplitPass())
 MODULE_PASS("hotcoldsplit", HotColdSplittingPass())
-MODULE_PASS("hwasan", HWAddressSanitizerPass(false, false))
-MODULE_PASS("khwasan", HWAddressSanitizerPass(true, true))
 MODULE_

[PATCH] D105974: [analyzer] Do not assume that all pointers have the same bitwidth as void*

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

I think you can create an upstream test case by looking at 
`clang/test/Analysis/ptr.cl`.
It uses amdgcn and address space 256, which seems to have a different pointer 
size compared to address space zero for that specific target.

So the test case (named with a .cl extension) would look like this (I don't 
think you really need to specify verde as target cpu):

  // RUN: %clang_analyze_cc1 -triple amdgcn-unknown-unknown -target-cpu verde 
-analyze -analyzer-checker=core %s
  
  void test(__attribute__((address_space(256))) int * p) {
__attribute__((address_space(256))) int * q = p-1;
if (q) {}
if (q) {} // crash
(void)q;
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105974

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

[PATCH] D113753: [Analyzer][Core] Better simplification in SimpleSValBuilder::evalBinOpNN

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

This seem to cause some weird results.

Given this input:

  bar(short k) {
k++;
for (short f = 0; f < k; f++)
  ;
(long)k << 16;
  }

we get

  > clang  --analyze --target=x86_64 'bbi-63538.c'
  bbi-63538.c:5:11: warning: The result of the left shift is undefined due to 
shifting '1' by '16', which is unrepresentable in the unsigned version of the 
return type 'long' [core.UndefinedBinaryOperatorResult]
(long)k << 16;
~~~ ^
  bbi-63538.c:5:11: warning: The result of the left shift is undefined due to 
shifting '2' by '16', which is unrepresentable in the unsigned version of the 
return type 'long' [core.UndefinedBinaryOperatorResult]
(long)k << 16;
~~~ ^
  bbi-63538.c:5:11: warning: The result of the left shift is undefined due to 
shifting '3' by '16', which is unrepresentable in the unsigned version of the 
return type 'long' [core.UndefinedBinaryOperatorResult]
(long)k << 16;
~~~ ^
  3 warnings generated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113753

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

[PATCH] D131255: Fix Wbitfield-constant-conversion on 1-bit signed bitfield

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

FWIW, just some things noticed when I examined some of the new warning that 
popped up after this patch:

https://github.com/llvm/llvm-project/issues/53253 mentioned that for example 
gcc complained about this. Although, as shown here 
https://godbolt.org/z/bq34Kexac there are some other differences that clang now 
complains already with -Wall, but that is not the case with gcc (I think one 
need to enable `-Wpedantic`  in gcc to get a warning about signed overflow).

A similar warning (when assigning -1 to an unsigned bitfield) can be given by 
both gcc and clang by using ` -Wsign-conversion` but that is not part of 
`-Wall` either. But maybe that is a totally different thing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131255

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

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.



Comment at: clang/lib/CodeGen/CGVTables.cpp:693
 return Int32Ty;
-  return Int8PtrTy;
+  return GlobalsInt8PtrTy;
 }

I noticed that we have some old fixes downstream that conflicts with the 
changes you've made here. I thought that perhaps we could get rid of those now 
when you've fixed the code upstream.

Isn't the VTable holding function pointers when not using the relative layout, 
and then this should be a pointer to the ProgramAddressSpace and not a pointer 
to the DefaultGlobalsAddressSpace?

Downstream we've been using a special `FnVoidPtrTy` here. Defined as 
`FnVoidPtrTy = Int8Ty->getPointerTo(DL.getProgramAddressSpace());`.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153092

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

[PATCH] D156733: Stop using legacy helpers indicating typed pointer types. NFC

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope created this revision.
Herald added subscribers: nlopes, Enna1, pmatos, asb, jeroen.dobbelaere, 
pengfei, hiraditya, jgravelle-google, sbc100, dschuff.
Herald added a project: All.
bjope requested review of this revision.
Herald added subscribers: cfe-commits, wangpc, aheejin.
Herald added projects: clang, LLVM.

Since we no longer support typed LLVM IR pointer types, the code can
be simplified into for example using PointerType::get directly instead
of using Type::getInt8PtrTy and Type::getInt32PtrTy etc.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156733

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGObjCGNU.cpp
  clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
  clang/tools/clang-linker-wrapper/OffloadWrapper.cpp
  clang/unittests/CodeGen/TBAAMetadataTest.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
  llvm/include/llvm/IR/IRBuilder.h
  llvm/lib/IR/Function.cpp
  llvm/lib/Linker/IRMover.cpp
  llvm/lib/Target/ARM/ARMFastISel.cpp
  llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
  llvm/lib/Target/X86/X86ISelLowering.cpp
  llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
  llvm/lib/Transforms/InstCombine/InstCombineInternal.h
  llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
  llvm/lib/Transforms/Scalar/GVN.cpp
  llvm/lib/Transforms/Utils/EntryExitInstrumenter.cpp
  llvm/unittests/Analysis/AliasAnalysisTest.cpp
  llvm/unittests/Analysis/AssumeBundleQueriesTest.cpp
  llvm/unittests/Analysis/PhiValuesTest.cpp
  llvm/unittests/Analysis/SparsePropagation.cpp
  llvm/unittests/Analysis/TBAATest.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  llvm/unittests/IR/BasicBlockTest.cpp
  llvm/unittests/IR/ConstantsTest.cpp
  llvm/unittests/IR/InstructionsTest.cpp
  llvm/unittests/IR/MetadataTest.cpp
  llvm/unittests/Transforms/Utils/CloningTest.cpp
  llvm/unittests/Transforms/Utils/LocalTest.cpp
  llvm/unittests/Transforms/Utils/ScalarEvolutionExpanderTest.cpp

Index: llvm/unittests/Transforms/Utils/ScalarEvolutionExpanderTest.cpp
===
--- llvm/unittests/Transforms/Utils/ScalarEvolutionExpanderTest.cpp
+++ llvm/unittests/Transforms/Utils/ScalarEvolutionExpanderTest.cpp
@@ -73,9 +73,9 @@
   // expansion when the value in ValueOffsetPair is a ptr and the offset
   // is not divisible by the elem type size of value.
   auto *I8Ty = Type::getInt8Ty(Context);
-  auto *I8PtrTy = Type::getInt8PtrTy(Context);
+  auto *I8PtrTy = PointerType::get(Context, 0);
   auto *I32Ty = Type::getInt32Ty(Context);
-  auto *I32PtrTy = Type::getInt32PtrTy(Context);
+  auto *I32PtrTy = PointerType::get(Context, 0);
   FunctionType *FTy =
   FunctionType::get(Type::getVoidTy(Context), std::vector(), false);
   Function *F = Function::Create(FTy, Function::ExternalLinkage, "f", M);
Index: llvm/unittests/Transforms/Utils/LocalTest.cpp
===
--- llvm/unittests/Transforms/Utils/LocalTest.cpp
+++ llvm/unittests/Transforms/Utils/LocalTest.cpp
@@ -154,7 +154,7 @@
   ASSERT_TRUE(Inst);
   auto *DII = dyn_cast(Inst);
   ASSERT_TRUE(DII);
-  Value *NewBase = Constant::getNullValue(Type::getInt32PtrTy(C));
+  Value *NewBase = Constant::getNullValue(PointerType::getUnqual(C));
   DIBuilder DIB(*M);
   replaceDbgDeclare(AI, NewBase, DIB, DIExpression::ApplyOffset, 0);
 
Index: llvm/unittests/Transforms/Utils/CloningTest.cpp
===
--- llvm/unittests/Transforms/Utils/CloningTest.cpp
+++ llvm/unittests/Transforms/Utils/CloningTest.cpp
@@ -137,7 +137,7 @@
 }
 
 TEST_F(CloneInstruction, Inbounds) {
-  V = new Argument(Type::getInt32PtrTy(context));
+  V = new Argument(PointerType::get(context, 0));
 
   Constant *Z = Constant::getNullValue(Type::getInt32Ty(context));
   std::vector ops;
@@ -161,8 +161,9 @@
 }
 
 TEST_F(CloneInstruction, Attributes) {
-  Type *ArgTy1[] = { Type::getInt32PtrTy(context) };
-  FunctionType *FT1 =  FunctionType::get(Type::getVoidTy(context), ArgTy1, false);
+  Type *ArgTy1[] = {PointerType::get(context, 0)};
+  FunctionType *FT1 =
+  FunctionType::get(Type::getVoidTy(context), ArgTy1, false);
 
   Function *F1 = Function::Create(FT1, Function::ExternalLinkage);
   BasicBlock *BB = BasicBlock::Create(context, "", F1);
@@ -187,8 +188,9 @@
 }
 
 TEST_F(CloneInstruction, CallingConvention) {
-  Type *ArgTy1[] = { Type::getInt32PtrTy(context) };
-  FunctionType *FT1 =  FunctionType::get(Type::getVoidTy(context), ArgTy1, false);
+  Type *ArgTy1[] = {PointerType::get(context, 0)};
+  FunctionType *FT1 =
+  FunctionType::get(Type::getVoidTy(context), ArgTy1, false);
 
   Function *F1 = Function::Create(FT1, Function::ExternalLinkage);
   F1->setCallingConv(CallingConv::Cold);
@@ -211,7 +213,7 @@
 }
 
 TEST_F(CloneInstruction, DuplicateInstructionsToSplit) {
-  Type *ArgTy1[] = {Type::getInt32PtrTy(context)};
+  Type *ArgTy1[] = {PointerType::get(context, 0)};

[PATCH] D156733: Stop using legacy helpers indicating typed pointer types. NFC

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope updated this revision to Diff 545783.
bjope added a comment.
Herald added a subscriber: ormris.

Replaced a couple of more occurances in this update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156733

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGObjCGNU.cpp
  clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
  clang/tools/clang-linker-wrapper/OffloadWrapper.cpp
  clang/unittests/CodeGen/TBAAMetadataTest.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
  llvm/include/llvm/IR/IRBuilder.h
  llvm/lib/IR/Function.cpp
  llvm/lib/Linker/IRMover.cpp
  llvm/lib/Target/ARM/ARMFastISel.cpp
  llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
  llvm/lib/Target/X86/X86ISelLowering.cpp
  llvm/lib/Transforms/IPO/CrossDSOCFI.cpp
  llvm/lib/Transforms/IPO/LowerTypeTests.cpp
  llvm/lib/Transforms/IPO/OpenMPOpt.cpp
  llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
  llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
  llvm/lib/Transforms/InstCombine/InstCombineInternal.h
  llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
  llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
  llvm/lib/Transforms/Scalar/GVN.cpp
  llvm/lib/Transforms/Utils/EntryExitInstrumenter.cpp
  llvm/tools/llvm-stress/llvm-stress.cpp
  llvm/unittests/Analysis/AliasAnalysisTest.cpp
  llvm/unittests/Analysis/AssumeBundleQueriesTest.cpp
  llvm/unittests/Analysis/PhiValuesTest.cpp
  llvm/unittests/Analysis/SparsePropagation.cpp
  llvm/unittests/Analysis/TBAATest.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  llvm/unittests/IR/BasicBlockTest.cpp
  llvm/unittests/IR/ConstantsTest.cpp
  llvm/unittests/IR/InstructionsTest.cpp
  llvm/unittests/IR/MetadataTest.cpp
  llvm/unittests/Transforms/Utils/CloningTest.cpp
  llvm/unittests/Transforms/Utils/LocalTest.cpp
  llvm/unittests/Transforms/Utils/ScalarEvolutionExpanderTest.cpp

Index: llvm/unittests/Transforms/Utils/ScalarEvolutionExpanderTest.cpp
===
--- llvm/unittests/Transforms/Utils/ScalarEvolutionExpanderTest.cpp
+++ llvm/unittests/Transforms/Utils/ScalarEvolutionExpanderTest.cpp
@@ -73,9 +73,9 @@
   // expansion when the value in ValueOffsetPair is a ptr and the offset
   // is not divisible by the elem type size of value.
   auto *I8Ty = Type::getInt8Ty(Context);
-  auto *I8PtrTy = Type::getInt8PtrTy(Context);
+  auto *I8PtrTy = PointerType::get(Context, 0);
   auto *I32Ty = Type::getInt32Ty(Context);
-  auto *I32PtrTy = Type::getInt32PtrTy(Context);
+  auto *I32PtrTy = PointerType::get(Context, 0);
   FunctionType *FTy =
   FunctionType::get(Type::getVoidTy(Context), std::vector(), false);
   Function *F = Function::Create(FTy, Function::ExternalLinkage, "f", M);
Index: llvm/unittests/Transforms/Utils/LocalTest.cpp
===
--- llvm/unittests/Transforms/Utils/LocalTest.cpp
+++ llvm/unittests/Transforms/Utils/LocalTest.cpp
@@ -154,7 +154,7 @@
   ASSERT_TRUE(Inst);
   auto *DII = dyn_cast(Inst);
   ASSERT_TRUE(DII);
-  Value *NewBase = Constant::getNullValue(Type::getInt32PtrTy(C));
+  Value *NewBase = Constant::getNullValue(PointerType::getUnqual(C));
   DIBuilder DIB(*M);
   replaceDbgDeclare(AI, NewBase, DIB, DIExpression::ApplyOffset, 0);
 
Index: llvm/unittests/Transforms/Utils/CloningTest.cpp
===
--- llvm/unittests/Transforms/Utils/CloningTest.cpp
+++ llvm/unittests/Transforms/Utils/CloningTest.cpp
@@ -137,7 +137,7 @@
 }
 
 TEST_F(CloneInstruction, Inbounds) {
-  V = new Argument(Type::getInt32PtrTy(context));
+  V = new Argument(PointerType::get(context, 0));
 
   Constant *Z = Constant::getNullValue(Type::getInt32Ty(context));
   std::vector ops;
@@ -161,8 +161,9 @@
 }
 
 TEST_F(CloneInstruction, Attributes) {
-  Type *ArgTy1[] = { Type::getInt32PtrTy(context) };
-  FunctionType *FT1 =  FunctionType::get(Type::getVoidTy(context), ArgTy1, false);
+  Type *ArgTy1[] = {PointerType::get(context, 0)};
+  FunctionType *FT1 =
+  FunctionType::get(Type::getVoidTy(context), ArgTy1, false);
 
   Function *F1 = Function::Create(FT1, Function::ExternalLinkage);
   BasicBlock *BB = BasicBlock::Create(context, "", F1);
@@ -187,8 +188,9 @@
 }
 
 TEST_F(CloneInstruction, CallingConvention) {
-  Type *ArgTy1[] = { Type::getInt32PtrTy(context) };
-  FunctionType *FT1 =  FunctionType::get(Type::getVoidTy(context), ArgTy1, false);
+  Type *ArgTy1[] = {PointerType::get(context, 0)};
+  FunctionType *FT1 =
+  FunctionType::get(Type::getVoidTy(context), ArgTy1, false);
 
   Function *F1 = Function::Create(FT1, Function::ExternalLinkage);
   F1->setCallingConv(CallingConv::Cold);
@@ -211,7 +213,7 @@
 }
 
 TEST_F(CloneInstruction, DuplicateInstructionsToSplit) {
-  Type *ArgTy1[] = {Type::getInt32PtrTy(context)};
+  Type *ArgTy1[] = {PointerType::get(context, 0)};
   FunctionType *FT = FunctionTy

[PATCH] D156733: Stop using legacy helpers indicating typed pointer types. NFC

[Bjorn Pettersson via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGfd05c34b18fb: Stop using legacy helpers indicating typed 
pointer types. NFC (authored by bjope).

Changed prior to commit:
  https://reviews.llvm.org/D156733?vs=545783&id=546377#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156733

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGObjCGNU.cpp
  clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
  clang/tools/clang-linker-wrapper/OffloadWrapper.cpp
  clang/unittests/CodeGen/TBAAMetadataTest.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
  llvm/include/llvm/IR/IRBuilder.h
  llvm/lib/IR/Function.cpp
  llvm/lib/Linker/IRMover.cpp
  llvm/lib/Target/ARM/ARMFastISel.cpp
  llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
  llvm/lib/Target/X86/X86ISelLowering.cpp
  llvm/lib/Transforms/IPO/CrossDSOCFI.cpp
  llvm/lib/Transforms/IPO/LowerTypeTests.cpp
  llvm/lib/Transforms/IPO/OpenMPOpt.cpp
  llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
  llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
  llvm/lib/Transforms/InstCombine/InstCombineInternal.h
  llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
  llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp
  llvm/lib/Transforms/Scalar/GVN.cpp
  llvm/lib/Transforms/Utils/EntryExitInstrumenter.cpp
  llvm/tools/llvm-stress/llvm-stress.cpp
  llvm/unittests/Analysis/AliasAnalysisTest.cpp
  llvm/unittests/Analysis/AssumeBundleQueriesTest.cpp
  llvm/unittests/Analysis/PhiValuesTest.cpp
  llvm/unittests/Analysis/SparsePropagation.cpp
  llvm/unittests/Analysis/TBAATest.cpp
  llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp
  llvm/unittests/IR/BasicBlockTest.cpp
  llvm/unittests/IR/ConstantsTest.cpp
  llvm/unittests/IR/InstructionsTest.cpp
  llvm/unittests/IR/MetadataTest.cpp
  llvm/unittests/Transforms/Utils/CloningTest.cpp
  llvm/unittests/Transforms/Utils/LocalTest.cpp
  llvm/unittests/Transforms/Utils/ScalarEvolutionExpanderTest.cpp

Index: llvm/unittests/Transforms/Utils/ScalarEvolutionExpanderTest.cpp
===
--- llvm/unittests/Transforms/Utils/ScalarEvolutionExpanderTest.cpp
+++ llvm/unittests/Transforms/Utils/ScalarEvolutionExpanderTest.cpp
@@ -73,9 +73,9 @@
   // expansion when the value in ValueOffsetPair is a ptr and the offset
   // is not divisible by the elem type size of value.
   auto *I8Ty = Type::getInt8Ty(Context);
-  auto *I8PtrTy = Type::getInt8PtrTy(Context);
+  auto *I8PtrTy = PointerType::get(Context, 0);
   auto *I32Ty = Type::getInt32Ty(Context);
-  auto *I32PtrTy = Type::getInt32PtrTy(Context);
+  auto *I32PtrTy = PointerType::get(Context, 0);
   FunctionType *FTy =
   FunctionType::get(Type::getVoidTy(Context), std::vector(), false);
   Function *F = Function::Create(FTy, Function::ExternalLinkage, "f", M);
Index: llvm/unittests/Transforms/Utils/LocalTest.cpp
===
--- llvm/unittests/Transforms/Utils/LocalTest.cpp
+++ llvm/unittests/Transforms/Utils/LocalTest.cpp
@@ -154,7 +154,7 @@
   ASSERT_TRUE(Inst);
   auto *DII = dyn_cast(Inst);
   ASSERT_TRUE(DII);
-  Value *NewBase = Constant::getNullValue(Type::getInt32PtrTy(C));
+  Value *NewBase = Constant::getNullValue(PointerType::getUnqual(C));
   DIBuilder DIB(*M);
   replaceDbgDeclare(AI, NewBase, DIB, DIExpression::ApplyOffset, 0);
 
Index: llvm/unittests/Transforms/Utils/CloningTest.cpp
===
--- llvm/unittests/Transforms/Utils/CloningTest.cpp
+++ llvm/unittests/Transforms/Utils/CloningTest.cpp
@@ -137,7 +137,7 @@
 }
 
 TEST_F(CloneInstruction, Inbounds) {
-  V = new Argument(Type::getInt32PtrTy(context));
+  V = new Argument(PointerType::get(context, 0));
 
   Constant *Z = Constant::getNullValue(Type::getInt32Ty(context));
   std::vector ops;
@@ -161,8 +161,9 @@
 }
 
 TEST_F(CloneInstruction, Attributes) {
-  Type *ArgTy1[] = { Type::getInt32PtrTy(context) };
-  FunctionType *FT1 =  FunctionType::get(Type::getVoidTy(context), ArgTy1, false);
+  Type *ArgTy1[] = {PointerType::get(context, 0)};
+  FunctionType *FT1 =
+  FunctionType::get(Type::getVoidTy(context), ArgTy1, false);
 
   Function *F1 = Function::Create(FT1, Function::ExternalLinkage);
   BasicBlock *BB = BasicBlock::Create(context, "", F1);
@@ -187,8 +188,9 @@
 }
 
 TEST_F(CloneInstruction, CallingConvention) {
-  Type *ArgTy1[] = { Type::getInt32PtrTy(context) };
-  FunctionType *FT1 =  FunctionType::get(Type::getVoidTy(context), ArgTy1, false);
+  Type *ArgTy1[] = {PointerType::get(context, 0)};
+  FunctionType *FT1 =
+  FunctionType::get(Type::getVoidTy(context), ArgTy1, false);
 
   Function *F1 = Function::Create(FT1, Function::ExternalLinkage);
   

[PATCH] D156911: [clang][CodeGen] Drop some typed pointer bitcasts

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope created this revision.
Herald added a subscriber: nlopes.
Herald added a project: All.
bjope requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156911

Files:
  clang/lib/CodeGen/CGBlocks.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp

Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2049,8 +2049,7 @@
NullConstant, Twine());
 CharUnits NullAlign = DestPtr.getAlignment();
 NullVariable->setAlignment(NullAlign.getAsAlign());
-Address SrcPtr(Builder.CreateBitCast(NullVariable, Builder.getInt8PtrTy()),
-   Builder.getInt8Ty(), NullAlign);
+Address SrcPtr(NullVariable, Builder.getInt8Ty(), NullAlign);
 
 if (vla) return emitNonZeroVLAInit(*this, Ty, DestPtr, SrcPtr, SizeVal);
 
Index: clang/lib/CodeGen/CGExprScalar.cpp
===
--- clang/lib/CodeGen/CGExprScalar.cpp
+++ clang/lib/CodeGen/CGExprScalar.cpp
@@ -3690,8 +3690,8 @@
 
 index = CGF.Builder.CreateMul(index, objectSize);
 
-Value *result = CGF.Builder.CreateBitCast(pointer, CGF.VoidPtrTy);
-result = CGF.Builder.CreateGEP(CGF.Int8Ty, result, index, "add.ptr");
+Value *result =
+CGF.Builder.CreateGEP(CGF.Int8Ty, pointer, index, "add.ptr");
 return CGF.Builder.CreateBitCast(result, pointer->getType());
   }
 
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -140,9 +140,8 @@
 auto *VectorTy = llvm::FixedVectorType::get(ArrayTy->getElementType(),
 ArrayTy->getNumElements());
 
-Result = Address(
-Builder.CreateBitCast(Result.getPointer(), VectorTy->getPointerTo()),
-VectorTy, Result.getAlignment(), KnownNonNull);
+Result = Address(Result.getPointer(), VectorTy, Result.getAlignment(),
+ KnownNonNull);
   }
   return Result;
 }
@@ -746,9 +745,8 @@
   llvm::Value *Min = Builder.getFalse();
   llvm::Value *NullIsUnknown = Builder.getFalse();
   llvm::Value *Dynamic = Builder.getFalse();
-  llvm::Value *CastAddr = Builder.CreateBitCast(Ptr, Int8PtrTy);
   llvm::Value *LargeEnough = Builder.CreateICmpUGE(
-  Builder.CreateCall(F, {CastAddr, Min, NullIsUnknown, Dynamic}), Size);
+  Builder.CreateCall(F, {Ptr, Min, NullIsUnknown, Dynamic}), Size);
   Checks.push_back(std::make_pair(LargeEnough, SanitizerKind::ObjectSize));
 }
   }
@@ -825,9 +823,7 @@
 
   // Load the vptr, and compute hash_16_bytes(TypeHash, vptr).
   llvm::Value *Low = llvm::ConstantInt::get(Int64Ty, TypeHash);
-  llvm::Type *VPtrTy = llvm::PointerType::get(IntPtrTy, 0);
-  Address VPtrAddr(Builder.CreateBitCast(Ptr, VPtrTy), IntPtrTy,
-   getPointerAlign());
+  Address VPtrAddr(Ptr, IntPtrTy, getPointerAlign());
   llvm::Value *VPtrVal = Builder.CreateLoad(VPtrAddr);
   llvm::Value *High = Builder.CreateZExt(VPtrVal, Int64Ty);
 
@@ -2492,14 +2488,6 @@
   }
 }
 
-static llvm::Value *
-EmitBitCastOfLValueToProperType(CodeGenFunction &CGF,
-llvm::Value *V, llvm::Type *IRType,
-StringRef Name = StringRef()) {
-  unsigned AS = cast(V->getType())->getAddressSpace();
-  return CGF.Builder.CreateBitCast(V, IRType->getPointerTo(AS), Name);
-}
-
 static LValue EmitThreadPrivateVarDeclLValue(
 CodeGenFunction &CGF, const VarDecl *VD, QualType T, Address Addr,
 llvm::Type *RealVarTy, SourceLocation Loc) {
@@ -2600,7 +2588,6 @@
 V = CGF.Builder.CreateThreadLocalAddress(V);
 
   llvm::Type *RealVarTy = CGF.getTypes().ConvertTypeForMem(VD->getType());
-  V = EmitBitCastOfLValueToProperType(CGF, V, RealVarTy);
   CharUnits Alignment = CGF.getContext().getDeclAlign(VD);
   Address Addr(V, RealVarTy, Alignment);
   // Emit reference to the private copy of the variable if it is an OpenMP
@@ -3421,8 +3408,7 @@
 "__cfi_slowpath_diag",
 llvm::FunctionType::get(VoidTy, {Int64Ty, Int8PtrTy, Int8PtrTy},
 false));
-CheckCall = Builder.CreateCall(
-SlowPathFn, {TypeId, Ptr, Builder.CreateBitCast(InfoPtr, Int8PtrTy)});
+CheckCall = Builder.CreateCall(SlowPathFn, {TypeId, Ptr, InfoPtr});
   } else {
 SlowPathFn = CGM.getModule().getOrInsertFunction(
 "__cfi_slowpath",
@@ -5365,8 +5351,7 @@
 AlignedCalleePtr = CalleePtr;
   }
 
-  llvm::Value *CalleePrefixStruct = Builder.CreateBitCast(
-  Align

[PATCH] D156911: [clang][CodeGen] Drop some typed pointer bitcasts

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope updated this revision to Diff 546921.
bjope added a comment.

Rebased+updated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156911

Files:
  clang/lib/CodeGen/CGBlocks.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/ItaniumCXXABI.cpp

Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -647,9 +647,7 @@
   // Apply the adjustment and cast back to the original struct type
   // for consistency.
   llvm::Value *This = ThisAddr.getPointer();
-  llvm::Value *Ptr = Builder.CreateBitCast(This, Builder.getInt8PtrTy());
-  Ptr = Builder.CreateInBoundsGEP(Builder.getInt8Ty(), Ptr, Adj);
-  This = Builder.CreateBitCast(Ptr, This->getType(), "this.adjusted");
+  This = Builder.CreateInBoundsGEP(Builder.getInt8Ty(), This, Adj);
   ThisPtrForCall = This;
 
   // Load the function pointer.
@@ -740,9 +738,8 @@
   ? llvm::Intrinsic::type_test
   : llvm::Intrinsic::public_type_test;
 
-CheckResult = Builder.CreateCall(
-CGM.getIntrinsic(IID),
-{Builder.CreateBitCast(VFPAddr, CGF.Int8PtrTy), TypeId});
+CheckResult =
+Builder.CreateCall(CGM.getIntrinsic(IID), {VFPAddr, TypeId});
   }
 
   if (CGM.getItaniumVTableContext().isRelativeLayout()) {
@@ -812,8 +809,6 @@
   };
 
   llvm::Value *Bit = Builder.getFalse();
-  llvm::Value *CastedNonVirtualFn =
-  Builder.CreateBitCast(NonVirtualFn, CGF.Int8PtrTy);
   for (const CXXRecordDecl *Base : CGM.getMostBaseClasses(RD)) {
 llvm::Metadata *MD = CGM.CreateMetadataIdentifierForType(
 getContext().getMemberPointerType(
@@ -824,13 +819,13 @@
 
 llvm::Value *TypeTest =
 Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::type_test),
-   {CastedNonVirtualFn, TypeId});
+   {NonVirtualFn, TypeId});
 Bit = Builder.CreateOr(Bit, TypeTest);
   }
 
   CGF.EmitCheck(std::make_pair(Bit, SanitizerKind::CFIMFCall),
 SanitizerHandler::CFICheckFail, StaticData,
-{CastedNonVirtualFn, llvm::UndefValue::get(CGF.IntPtrTy)});
+{NonVirtualFn, llvm::UndefValue::get(CGF.IntPtrTy)});
 
   FnNonVirtual = Builder.GetInsertBlock();
 }
@@ -1253,8 +1248,7 @@
 CGF.getPointerAlign());
 
 // Apply the offset.
-llvm::Value *CompletePtr =
-  CGF.Builder.CreateBitCast(Ptr.getPointer(), CGF.Int8PtrTy);
+llvm::Value *CompletePtr = Ptr.getPointer();
 CompletePtr =
 CGF.Builder.CreateInBoundsGEP(CGF.Int8Ty, CompletePtr, Offset);
 
@@ -1454,7 +1448,6 @@
 
   if (CGM.getItaniumVTableContext().isRelativeLayout()) {
 // Load the type info.
-Value = CGF.Builder.CreateBitCast(Value, CGM.Int8PtrTy);
 Value = CGF.Builder.CreateCall(
 CGM.getIntrinsic(llvm::Intrinsic::load_relative, {CGM.Int32Ty}),
 {Value, llvm::ConstantInt::get(CGM.Int32Ty, -4)});
@@ -2211,8 +2204,7 @@
NonVirtualAdjustment);
   }
 
-  // Cast back to the original type.
-  return CGF.Builder.CreateBitCast(ResultPtr, InitialPtr.getType());
+  return ResultPtr;
 }
 
 llvm::Value *ItaniumCXXABI::performThisAdjustment(CodeGenFunction &CGF,
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2049,8 +2049,7 @@
NullConstant, Twine());
 CharUnits NullAlign = DestPtr.getAlignment();
 NullVariable->setAlignment(NullAlign.getAsAlign());
-Address SrcPtr(Builder.CreateBitCast(NullVariable, Builder.getInt8PtrTy()),
-   Builder.getInt8Ty(), NullAlign);
+Address SrcPtr(NullVariable, Builder.getInt8Ty(), NullAlign);
 
 if (vla) return emitNonZeroVLAInit(*this, Ty, DestPtr, SrcPtr, SizeVal);
 
Index: clang/lib/CodeGen/CGExprScalar.cpp
===
--- clang/lib/CodeGen/CGExprScalar.cpp
+++ clang/lib/CodeGen/CGExprScalar.cpp
@@ -3690,8 +3690,8 @@
 
 index = CGF.Builder.CreateMul(index, objectSize);
 
-Value *result = CGF.Builder.CreateBitCast(pointer, CGF.VoidPtrTy);
-result = CGF.Builder.CreateGEP(CGF.Int8Ty, result, index, "add.ptr");
+Value *result =
+CGF.Builder.CreateGEP(CGF.Int8Ty, pointer, index, "add.ptr");
 return CGF.Builder.CreateBitCast(result, pointer->getType());
   }
 
Index: clang/lib/CodeGen/CGExp

[PATCH] D156911: [clang][CodeGen] Drop some typed pointer bitcasts

[Bjorn Pettersson via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG2bdc86484d03: [clang][CodeGen] Drop some typed pointer 
bitcasts (authored by bjope).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156911

Files:
  clang/lib/CodeGen/CGBlocks.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGClass.cpp
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/ItaniumCXXABI.cpp

Index: clang/lib/CodeGen/ItaniumCXXABI.cpp
===
--- clang/lib/CodeGen/ItaniumCXXABI.cpp
+++ clang/lib/CodeGen/ItaniumCXXABI.cpp
@@ -647,9 +647,7 @@
   // Apply the adjustment and cast back to the original struct type
   // for consistency.
   llvm::Value *This = ThisAddr.getPointer();
-  llvm::Value *Ptr = Builder.CreateBitCast(This, Builder.getInt8PtrTy());
-  Ptr = Builder.CreateInBoundsGEP(Builder.getInt8Ty(), Ptr, Adj);
-  This = Builder.CreateBitCast(Ptr, This->getType(), "this.adjusted");
+  This = Builder.CreateInBoundsGEP(Builder.getInt8Ty(), This, Adj);
   ThisPtrForCall = This;
 
   // Load the function pointer.
@@ -740,9 +738,8 @@
   ? llvm::Intrinsic::type_test
   : llvm::Intrinsic::public_type_test;
 
-CheckResult = Builder.CreateCall(
-CGM.getIntrinsic(IID),
-{Builder.CreateBitCast(VFPAddr, CGF.Int8PtrTy), TypeId});
+CheckResult =
+Builder.CreateCall(CGM.getIntrinsic(IID), {VFPAddr, TypeId});
   }
 
   if (CGM.getItaniumVTableContext().isRelativeLayout()) {
@@ -812,8 +809,6 @@
   };
 
   llvm::Value *Bit = Builder.getFalse();
-  llvm::Value *CastedNonVirtualFn =
-  Builder.CreateBitCast(NonVirtualFn, CGF.Int8PtrTy);
   for (const CXXRecordDecl *Base : CGM.getMostBaseClasses(RD)) {
 llvm::Metadata *MD = CGM.CreateMetadataIdentifierForType(
 getContext().getMemberPointerType(
@@ -824,13 +819,13 @@
 
 llvm::Value *TypeTest =
 Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::type_test),
-   {CastedNonVirtualFn, TypeId});
+   {NonVirtualFn, TypeId});
 Bit = Builder.CreateOr(Bit, TypeTest);
   }
 
   CGF.EmitCheck(std::make_pair(Bit, SanitizerKind::CFIMFCall),
 SanitizerHandler::CFICheckFail, StaticData,
-{CastedNonVirtualFn, llvm::UndefValue::get(CGF.IntPtrTy)});
+{NonVirtualFn, llvm::UndefValue::get(CGF.IntPtrTy)});
 
   FnNonVirtual = Builder.GetInsertBlock();
 }
@@ -1253,8 +1248,7 @@
 CGF.getPointerAlign());
 
 // Apply the offset.
-llvm::Value *CompletePtr =
-  CGF.Builder.CreateBitCast(Ptr.getPointer(), CGF.Int8PtrTy);
+llvm::Value *CompletePtr = Ptr.getPointer();
 CompletePtr =
 CGF.Builder.CreateInBoundsGEP(CGF.Int8Ty, CompletePtr, Offset);
 
@@ -1454,7 +1448,6 @@
 
   if (CGM.getItaniumVTableContext().isRelativeLayout()) {
 // Load the type info.
-Value = CGF.Builder.CreateBitCast(Value, CGM.Int8PtrTy);
 Value = CGF.Builder.CreateCall(
 CGM.getIntrinsic(llvm::Intrinsic::load_relative, {CGM.Int32Ty}),
 {Value, llvm::ConstantInt::get(CGM.Int32Ty, -4)});
@@ -2211,8 +2204,7 @@
NonVirtualAdjustment);
   }
 
-  // Cast back to the original type.
-  return CGF.Builder.CreateBitCast(ResultPtr, InitialPtr.getType());
+  return ResultPtr;
 }
 
 llvm::Value *ItaniumCXXABI::performThisAdjustment(CodeGenFunction &CGF,
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2049,8 +2049,7 @@
NullConstant, Twine());
 CharUnits NullAlign = DestPtr.getAlignment();
 NullVariable->setAlignment(NullAlign.getAsAlign());
-Address SrcPtr(Builder.CreateBitCast(NullVariable, Builder.getInt8PtrTy()),
-   Builder.getInt8Ty(), NullAlign);
+Address SrcPtr(NullVariable, Builder.getInt8Ty(), NullAlign);
 
 if (vla) return emitNonZeroVLAInit(*this, Ty, DestPtr, SrcPtr, SizeVal);
 
Index: clang/lib/CodeGen/CGExprScalar.cpp
===
--- clang/lib/CodeGen/CGExprScalar.cpp
+++ clang/lib/CodeGen/CGExprScalar.cpp
@@ -3690,8 +3690,8 @@
 
 index = CGF.Builder.CreateMul(index, objectSize);
 
-Value *result = CGF.Builder.CreateBitCast(pointer, CGF.VoidPtrTy);
-result = CGF.Builder.CreateGEP(CGF

[PATCH] D157550: [clang] Drop some references to typed pointers (getInt8PtrTy). NFC

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope created this revision.
Herald added a subscriber: ChuanqiXu.
Herald added a project: All.
bjope requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157550

Files:
  clang/lib/CodeGen/CGBlocks.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCoroutine.cpp
  clang/lib/CodeGen/CGOpenCLRuntime.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp

Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -4500,7 +4500,7 @@
 
   return llvm::ConstantExpr::getBitCast(
   llvm::NoCFIValue::get(F),
-  llvm::Type::getInt8PtrTy(VMContext, F->getAddressSpace()));
+  llvm::PointerType::get(VMContext, F->getAddressSpace()));
 }
 
 static const FunctionDecl *
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2504,7 +2504,7 @@
   // FIXME We create a new bitcast for every annotation because that's what
   // llvm-gcc was doing.
   unsigned AS = V->getType()->getPointerAddressSpace();
-  llvm::Type *I8PtrTy = Builder.getInt8PtrTy(AS);
+  llvm::Type *I8PtrTy = Builder.getPtrTy(AS);
   for (const auto *I : D->specific_attrs())
 EmitAnnotationCall(CGM.getIntrinsic(llvm::Intrinsic::var_annotation,
 {I8PtrTy, CGM.ConstGlobalsPtrTy}),
Index: clang/lib/CodeGen/CGOpenCLRuntime.cpp
===
--- clang/lib/CodeGen/CGOpenCLRuntime.cpp
+++ clang/lib/CodeGen/CGOpenCLRuntime.cpp
@@ -134,7 +134,7 @@
 
 llvm::PointerType *CGOpenCLRuntime::getGenericVoidPointerType() {
   assert(CGM.getLangOpts().OpenCL);
-  return llvm::IntegerType::getInt8PtrTy(
+  return llvm::PointerType::get(
   CGM.getLLVMContext(),
   CGM.getContext().getTargetAddressSpace(LangAS::opencl_generic));
 }
Index: clang/lib/CodeGen/CGCoroutine.cpp
===
--- clang/lib/CodeGen/CGCoroutine.cpp
+++ clang/lib/CodeGen/CGCoroutine.cpp
@@ -594,7 +594,7 @@
 }
 
 void CodeGenFunction::EmitCoroutineBody(const CoroutineBodyStmt &S) {
-  auto *NullPtr = llvm::ConstantPointerNull::get(Builder.getInt8PtrTy());
+  auto *NullPtr = llvm::ConstantPointerNull::get(Builder.getPtrTy());
   auto &TI = CGM.getContext().getTargetInfo();
   unsigned NewAlign = TI.getNewAlign() / TI.getCharWidth();
 
@@ -783,7 +783,7 @@
 }
 CGM.Error(E->getBeginLoc(), "this builtin expect that __builtin_coro_begin "
 "has been used earlier in this function");
-auto *NullPtr = llvm::ConstantPointerNull::get(Builder.getInt8PtrTy());
+auto *NullPtr = llvm::ConstantPointerNull::get(Builder.getPtrTy());
 return RValue::get(NullPtr);
   }
   case llvm::Intrinsic::coro_size: {
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -5009,7 +5009,7 @@
 unsigned NumArgs = E->getNumArgs();
 
 llvm::Type *QueueTy = ConvertType(getContext().OCLQueueTy);
-llvm::Type *GenericVoidPtrTy = Builder.getInt8PtrTy(
+llvm::Type *GenericVoidPtrTy = Builder.getPtrTy(
 getContext().getTargetAddressSpace(LangAS::opencl_generic));
 
 llvm::Value *Queue = EmitScalarExpr(E->getArg(0));
@@ -5187,7 +5187,7 @@
   // OpenCL v2.0 s6.13.17.6 - Kernel query functions need bitcast of block
   // parameter.
   case Builtin::BIget_kernel_work_group_size: {
-llvm::Type *GenericVoidPtrTy = Builder.getInt8PtrTy(
+llvm::Type *GenericVoidPtrTy = Builder.getPtrTy(
 getContext().getTargetAddressSpace(LangAS::opencl_generic));
 auto Info =
 CGM.getOpenCLRuntime().emitOpenCLEnqueuedBlock(*this, E->getArg(0));
@@ -5202,7 +5202,7 @@
 {Kernel, Arg}));
   }
   case Builtin::BIget_kernel_preferred_work_group_size_multiple: {
-llvm::Type *GenericVoidPtrTy = Builder.getInt8PtrTy(
+llvm::Type *GenericVoidPtrTy = Builder.getPtrTy(
 getContext().getTargetAddressSpace(LangAS::opencl_generic));
 auto Info =
 CGM.getOpenCLRuntime().emitOpenCLEnqueuedBlock(*this, E->getArg(0));
@@ -5218,7 +5218,7 @@
   }
   case Builtin::BIget_kernel_max_sub_group_size_for_ndrange:
   case Builtin::BIget_kernel_sub_group_count_for_ndrange: {
-llvm::Type *GenericVoidPtrTy = Builder.getInt8PtrTy(
+llvm::Type *GenericVoidPtrTy = Builder.getPtrTy(
 getContext().getTargetAddressSpace(LangAS::opencl_generic));
 LValue NDRangeL = EmitAggExprToLValue(E->getArg(0));
 llvm::Value *NDRange = NDRangeL.getAddress(*this).getPointer();
Index: clang/lib/CodeGen/CGBlocks.cpp
=

[PATCH] D157550: [clang] Drop some references to typed pointers (getInt8PtrTy). NFC

[Bjorn Pettersson via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd03f4177dfbb: [clang] Drop some references to typed pointers 
(getInt8PtrTy). NFC (authored by bjope).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157550

Files:
  clang/lib/CodeGen/CGBlocks.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGCoroutine.cpp
  clang/lib/CodeGen/CGOpenCLRuntime.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp

Index: clang/lib/CodeGen/CodeGenModule.cpp
===
--- clang/lib/CodeGen/CodeGenModule.cpp
+++ clang/lib/CodeGen/CodeGenModule.cpp
@@ -4500,7 +4500,7 @@
 
   return llvm::ConstantExpr::getBitCast(
   llvm::NoCFIValue::get(F),
-  llvm::Type::getInt8PtrTy(VMContext, F->getAddressSpace()));
+  llvm::PointerType::get(VMContext, F->getAddressSpace()));
 }
 
 static const FunctionDecl *
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -2504,7 +2504,7 @@
   // FIXME We create a new bitcast for every annotation because that's what
   // llvm-gcc was doing.
   unsigned AS = V->getType()->getPointerAddressSpace();
-  llvm::Type *I8PtrTy = Builder.getInt8PtrTy(AS);
+  llvm::Type *I8PtrTy = Builder.getPtrTy(AS);
   for (const auto *I : D->specific_attrs())
 EmitAnnotationCall(CGM.getIntrinsic(llvm::Intrinsic::var_annotation,
 {I8PtrTy, CGM.ConstGlobalsPtrTy}),
Index: clang/lib/CodeGen/CGOpenCLRuntime.cpp
===
--- clang/lib/CodeGen/CGOpenCLRuntime.cpp
+++ clang/lib/CodeGen/CGOpenCLRuntime.cpp
@@ -134,7 +134,7 @@
 
 llvm::PointerType *CGOpenCLRuntime::getGenericVoidPointerType() {
   assert(CGM.getLangOpts().OpenCL);
-  return llvm::IntegerType::getInt8PtrTy(
+  return llvm::PointerType::get(
   CGM.getLLVMContext(),
   CGM.getContext().getTargetAddressSpace(LangAS::opencl_generic));
 }
Index: clang/lib/CodeGen/CGCoroutine.cpp
===
--- clang/lib/CodeGen/CGCoroutine.cpp
+++ clang/lib/CodeGen/CGCoroutine.cpp
@@ -594,7 +594,7 @@
 }
 
 void CodeGenFunction::EmitCoroutineBody(const CoroutineBodyStmt &S) {
-  auto *NullPtr = llvm::ConstantPointerNull::get(Builder.getInt8PtrTy());
+  auto *NullPtr = llvm::ConstantPointerNull::get(Builder.getPtrTy());
   auto &TI = CGM.getContext().getTargetInfo();
   unsigned NewAlign = TI.getNewAlign() / TI.getCharWidth();
 
@@ -783,7 +783,7 @@
 }
 CGM.Error(E->getBeginLoc(), "this builtin expect that __builtin_coro_begin "
 "has been used earlier in this function");
-auto *NullPtr = llvm::ConstantPointerNull::get(Builder.getInt8PtrTy());
+auto *NullPtr = llvm::ConstantPointerNull::get(Builder.getPtrTy());
 return RValue::get(NullPtr);
   }
   case llvm::Intrinsic::coro_size: {
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -5010,7 +5010,7 @@
 unsigned NumArgs = E->getNumArgs();
 
 llvm::Type *QueueTy = ConvertType(getContext().OCLQueueTy);
-llvm::Type *GenericVoidPtrTy = Builder.getInt8PtrTy(
+llvm::Type *GenericVoidPtrTy = Builder.getPtrTy(
 getContext().getTargetAddressSpace(LangAS::opencl_generic));
 
 llvm::Value *Queue = EmitScalarExpr(E->getArg(0));
@@ -5188,7 +5188,7 @@
   // OpenCL v2.0 s6.13.17.6 - Kernel query functions need bitcast of block
   // parameter.
   case Builtin::BIget_kernel_work_group_size: {
-llvm::Type *GenericVoidPtrTy = Builder.getInt8PtrTy(
+llvm::Type *GenericVoidPtrTy = Builder.getPtrTy(
 getContext().getTargetAddressSpace(LangAS::opencl_generic));
 auto Info =
 CGM.getOpenCLRuntime().emitOpenCLEnqueuedBlock(*this, E->getArg(0));
@@ -5203,7 +5203,7 @@
 {Kernel, Arg}));
   }
   case Builtin::BIget_kernel_preferred_work_group_size_multiple: {
-llvm::Type *GenericVoidPtrTy = Builder.getInt8PtrTy(
+llvm::Type *GenericVoidPtrTy = Builder.getPtrTy(
 getContext().getTargetAddressSpace(LangAS::opencl_generic));
 auto Info =
 CGM.getOpenCLRuntime().emitOpenCLEnqueuedBlock(*this, E->getArg(0));
@@ -5219,7 +5219,7 @@
   }
   case Builtin::BIget_kernel_max_sub_group_size_for_ndrange:
   case Builtin::BIget_kernel_sub_group_count_for_ndrange: {
-llvm::Type *GenericVoidPtrTy = Builder.getInt8PtrTy(
+llvm::Type *GenericVoidPtrTy = Builder.getPtrTy(
 getContext().getTargetAddressSpace(LangAS::opencl_generic));
 LValue NDRangeL = EmitAggExprToLValue(E->ge

[PATCH] D76533: [clangd] Skip ClangdVFSTest.TestStackOverflow when address sanitizer is used

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope created this revision.
bjope added reviewers: sammccall, Dmitry.Kozhevnikov.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous, 
MaskRay, ilya-biryukov.
Herald added a project: clang.
bjope added a comment.
bjope added subscribers: uabelho, dstenb.

I don't know if this is a good solution. But we've had problems with our 
downstream bots that builds llvm with asan, and runs check-all, for awhile 
(probably since we merged https://reviews.llvm.org/D50993).


The ClangdVFSTest.TestStackOverflow unittest does not seem to work
when building the test with address sanitizer activated. Afaict the
goal is to get a constexpr depth error, rather than stack overflow.
But with asan instrumentation we do get stack overflow complaints
from asan. As a workaround this patch simply disables the test case,
when being built with address sanitizer activated.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76533

Files:
  clang-tools-extra/clangd/unittests/ClangdTests.cpp


Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -1083,6 +1083,9 @@
 Field(&CodeCompletion::Scope, "ns::";
 }
 
+// Tests fails when built with asan due to stack overflow. So skip running the
+// test as a workaround.
+#if !defined(__has_feature) || !__has_feature(address_sanitizer)
 TEST_F(ClangdVFSTest, TestStackOverflow) {
   MockFSProvider FS;
   ErrorCheckingCallbacks DiagConsumer;
@@ -1103,6 +1106,7 @@
   // overflow
   EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags());
 }
+#endif
 
 } // namespace
 } // namespace clangd


Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -1083,6 +1083,9 @@
 Field(&CodeCompletion::Scope, "ns::";
 }
 
+// Tests fails when built with asan due to stack overflow. So skip running the
+// test as a workaround.
+#if !defined(__has_feature) || !__has_feature(address_sanitizer)
 TEST_F(ClangdVFSTest, TestStackOverflow) {
   MockFSProvider FS;
   ErrorCheckingCallbacks DiagConsumer;
@@ -1103,6 +1106,7 @@
   // overflow
   EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags());
 }
+#endif
 
 } // namespace
 } // namespace clangd
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76533: [clangd] Skip ClangdVFSTest.TestStackOverflow when address sanitizer is used

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

I don't know if this is a good solution. But we've had problems with our 
downstream bots that builds llvm with asan, and runs check-all, for awhile 
(probably since we merged https://reviews.llvm.org/D50993).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76533



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

[PATCH] D76533: [clangd] Skip ClangdVFSTest.TestStackOverflow when address sanitizer is used

[Bjorn Pettersson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc6d799156a0a: [clangd] Skip ClangdVFSTest.TestStackOverflow 
when address sanitizer is used (authored by bjope).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76533

Files:
  clang-tools-extra/clangd/unittests/ClangdTests.cpp


Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -1083,6 +1083,9 @@
 Field(&CodeCompletion::Scope, "ns::";
 }
 
+// Tests fails when built with asan due to stack overflow. So skip running the
+// test as a workaround.
+#if !defined(__has_feature) || !__has_feature(address_sanitizer)
 TEST_F(ClangdVFSTest, TestStackOverflow) {
   MockFSProvider FS;
   ErrorCheckingCallbacks DiagConsumer;
@@ -1103,6 +1106,7 @@
   // overflow
   EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags());
 }
+#endif
 
 } // namespace
 } // namespace clangd


Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -1083,6 +1083,9 @@
 Field(&CodeCompletion::Scope, "ns::";
 }
 
+// Tests fails when built with asan due to stack overflow. So skip running the
+// test as a workaround.
+#if !defined(__has_feature) || !__has_feature(address_sanitizer)
 TEST_F(ClangdVFSTest, TestStackOverflow) {
   MockFSProvider FS;
   ErrorCheckingCallbacks DiagConsumer;
@@ -1103,6 +1106,7 @@
   // overflow
   EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags());
 }
+#endif
 
 } // namespace
 } // namespace clangd
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D70203: [AST] Attach comment in `/** doc */ typedef struct A {} B` to B as well as A.

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

Hi @sammccall . 
Just a heads up. Looks like this might have caused: 
https://bugs.llvm.org/show_bug.cgi?id=44143 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70203



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

[PATCH] D69825: [Clang][Driver] Re-use the calling process instead of creating a new process for the cc1 invocation

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

When I build the clang on trunk, using clang 8.0 with asan enabled, and then 
run the clang lit tests I see lots of failures in Clang :: 
InterfaceStubs/driver-test.c and Clang :: InterfaceStubs/driver-test2.c (and 
maybe the faults I get in Clang :: Driver/cl-showfilenames.c comes from this 
patch as well).

The failures go away if I revert this patch (commit 
b4a99a061f517e60985667e39519f601 
). To make 
the revert I also needed to revert commit 
8e5018e990b701391e6c33ba85b012343df67272 
 which is 
based on this patch.

Examples of failures:

  ==130643==ERROR: LeakSanitizer: detected memory leaks
  
  Direct leak of 192 byte(s) in 3 object(s) allocated from:
  #0 0xd0ea38 in operator new(unsigned long) 
/workspace/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:106
  #1 0x8153b36 in make_unique 
/proj/flexasic/app/llvm/8.0/bin/../include/c++/v1/memory:3132:28
  #2 0x8153b36 in CreateFrontendBaseAction 
/workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:68
  #3 0x8153b36 in clang::CreateFrontendAction(clang::CompilerInstance&) 
/workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:137
  #4 0x8156a22 in 
clang::ExecuteCompilerInvocation(clang::CompilerInstance*) 
/workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:287:39
  #5 0xd23349 in cc1_main(llvm::ArrayRef, char const*, void*) 
/workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/tools/driver/cc1_main.cpp:239:15
  #6 0x7a0082e in operator() 
/workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Driver/Job.cpp:402:34
  #7 0x7a0082e in void llvm::function_ref::callback_fn
 >, std::__1::basic_string, 
std::__1::allocator >*, bool*) const::$_1>(long) 
/workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../include/llvm/ADT/STLExtras.h:108
  #8 0x6634feb in operator() 
/workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../include/llvm/ADT/STLExtras.h:125:12
  #9 0x6634feb in 
llvm::CrashRecoveryContext::RunSafely(llvm::function_ref) 
/workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../lib/Support/CrashRecoveryContext.cpp:396
  #10 0x79fefaf in 
clang::driver::CC1Command::Execute(llvm::ArrayRef
 >, std::__1::basic_string, 
std::__1::allocator >*, bool*) const 
/workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Driver/Job.cpp:402:12
  #11 0x795265d in 
clang::driver::Compilation::ExecuteCommand(clang::driver::Command const&, 
clang::driver::Command const*&) const 
/workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Driver/Compilation.cpp:182:15
  #12 0x7953922 in 
clang::driver::Compilation::ExecuteJobs(clang::driver::JobList const&, 
llvm::SmallVectorImpl >&) 
const 
/workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Driver/Compilation.cpp:233:19
  #13 0x79a4d0e in 
clang::driver::Driver::ExecuteCompilation(clang::driver::Compilation&, 
llvm::SmallVectorImpl >&) 
/workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Driver/Driver.cpp:1473:5
  #14 0xd19eac in main 
/workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/tools/driver/driver.cpp:493:21
  #15 0x7f882be4e544 in __libc_start_main (/lib64/libc.so.6+0x22544)
  
  Direct leak of 184 byte(s) in 1 object(s) allocated from:
  #0 0xd0ea38 in operator new(unsigned long) 
/workspace/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:106
  #1 0x81537b4 in make_unique 
/proj/flexasic/app/llvm/8.0/bin/../include/c++/v1/memory:3132:28
  #2 0x81537b4 in CreateFrontendBaseAction 
/workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:52
  #3 0x81537b4 in clang::CreateFrontendAction(clang::CompilerInstance&) 
/workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:137
  #4 0x8156a22 in 
clang::ExecuteCompilerInvocation(clang::CompilerInstance*) 
/workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:287:39
  #5 0xd23349 in cc1_main(llvm::ArrayRef, char const*, void*) 
/workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/tools/driver/cc1_main.cpp:239:15
  #6 0x7a0082e in operator() 
/workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../../clang/lib/Driver/Job.cpp:402:34
  #7 0x7a0082e in void llvm::function_ref::callback_fn
 >, std::__1::basic_string, 
std::__1::allocator >*, bool*) const::$_1>(long) 
/workspace//llvm-master-sanitize-asan/llvm/build-all-asan/../include/llvm/ADT/STLExtras.h:108

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added a comment.

I get errors when compiling due to this commit:

  FAILED: 
tools/clang/lib/Tooling/CMakeFiles/clangTooling.dir/StandaloneExecution.cpp.o 
  /repo/app/clang/3.6/bin/clang++  -march=corei7  -DGTEST_HAS_RTTI=0 -D_DEBUG 
-D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS 
-D__STDC_LIMIT_MACROS -Itools/clang/lib/Tooling -I../tools/clang/lib/Tooling 
-I../tools/clang/include -Itools/clang/include -Iinclude -I../include 
-I/repo/app/valgrind/3.11.0/include  -fPIC -fvisibility-inlines-hidden -Werror 
-Werror=date-time -std=c++11 -Wall -W -Wno-unused-parameter -Wwrite-strings 
-Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long 
-Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor 
-Wstring-conversion -fcolor-diagnostics -ffunction-sections -fdata-sections 
-fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3-UNDEBUG  
-fno-exceptions -fno-rtti -MMD -MT 
tools/clang/lib/Tooling/CMakeFiles/clangTooling.dir/StandaloneExecution.cpp.o 
-MF 
tools/clang/lib/Tooling/CMakeFiles/clangTooling.dir/StandaloneExecution.cpp.o.d 
-o 
tools/clang/lib/Tooling/CMakeFiles/clangTooling.dir/StandaloneExecution.cpp.o 
-c ../tools/clang/lib/Tooling/StandaloneExecution.cpp
  ../tools/clang/lib/Tooling/StandaloneExecution.cpp:65:11: error: unused 
variable 'Ret' [-Werror,-Wunused-variable]
if (int Ret = Tool.run(Action.first.get()))
^
  1 error generated.


Repository:
  rL LLVM

https://reviews.llvm.org/D34272



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