[PATCH] D126689: [IR] Enable opaque pointers by default

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D126689#4654124 , @uabelho wrote:

> @nikic: Thanks, nothing to do there then even if I'm surprised.
>
> I'm not sure but I *think* that llvm-reduce may have some problems with this. 
> For my out of tree target I recently saw a case where llvm-reduced crashed 
> with
>
>   llvm-reduce: ../tools/llvm-reduce/deltas/ReduceOperandsToArgs.cpp:64: void 
> replaceFunctionCalls(llvm::Function *, llvm::Function *): Assertion 
> `CI->getCalledFunction() == OldF' failed.
>
> and when I looked at the reduced result so far, I saw a call where parameters 
> didn't match the declaration. So I guess it may now reduce in ways that it 
> unexpected for it and then crash.

Can you please file an issue for the llvm-reduce bug? I just took a quick look 
at the code, and it indeed has a mismatch in checks between 
canReplaceFunction() and replaceFunctionCalls() -- the conditions in both need 
to be the same, but aren't.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126689

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

[PATCH] D126689: [IR] Enable opaque pointers by default

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

@uabelho The call is well-defined from an LLVM IR perspective, so the verifier 
cannot reject it, just as it can't reject calling convention or ABI attribute 
mismatch. In this specific case, the call is likely undefined behavior, but 
that is not generally the case just because there is a signature mismatch. When 
exactly this is UB is still something of an open question (and I believe 
currently effectively boils down to "does the signature match after lowering or 
not").


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126689

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

[PATCH] D155688: [PATCH] [llvm] [InstCombine] Canonicalise ADD+GEP

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D155688#4653520 , @fiigii wrote:

>> The reverse transform is only done if A + B simplifies.
>
> Looks like`simplifyAddInst` may give add expressions, so I guess this patch 
> may make IC run into infinite loops.

simplifyAddInst can return an add instruction, but it will be an existing one. 
It will never introduce a new one. So I'm not sure how this would result in 
infinite loops?

In D155688#4653629 , @d-smirnov wrote:

> We have some improvements with the patch, most notable: 549.fotonik_3d 
> improves about 6%. 
> @nikic Should we revert the patch and try another location for it (in LICM 
> pass, as you previously suggested)?

I don't think we have cause to revert just yet, as we're not aware of any 
//specific// issues.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155688

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

[PATCH] D155688: [PATCH] [llvm] [InstCombine] Canonicalise ADD+GEP

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D155688#4653347 , @fiigii wrote:

> How does this patch work with `visitGEPOfGEP` that does a reverse 
> transformation?
>
>   // Replace: gep (gep %P, long B), long A, ...
>   // With:T = long A+B; gep %P, T, ...

The reverse transform is only done if `A + B` simplifies.



By the way, this change did cause some code size regressions: 
http://llvm-compile-time-tracker.com/compare.php?from=a16f6462d756804276d4b39267b3c19bcd6949fe&to=e13bed4c5f3544c076ce57e36d9a11eefa5a7815&stat=size-text

The one that stood out to me is that btGjkEpa2.cpp from bullet has become 13% 
larger.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155688

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

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

FYI this causes some compile-time regression: 
http://llvm-compile-time-tracker.com/compare.php?from=bc7d88faf1a595ab59952a2054418cdd0d98&to=af4751738db89a142a8880c782d12d4201b222a8&stat=instructions:u
 About 0.7% for C++ code at `O0`. Sample for a file with >2% would be 
btSoftSoftCollisionAlgorithm.cpp.

Not sure to what degree this is expected or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140828

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

[PATCH] D155688: [PATCH] [llvm] [InstCombine] Canonicalise ADD+GEP

[Nikita Popov via Phabricator via cfe-commits]

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

LGTM

We should give this a try, but I think there is a fairly large chance that this 
will cause regressions somewhere and a more targeted change may be necessary 
(e.g. only do this for loop-invariants in LICM).




Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2334-2335
+  Value *Ptr = GEP.getPointerOperand();
+  auto *NewPtr = GetElementPtrInst::Create(GEP.getResultElementType(), Ptr,
+   Idx1, "", &GEP);
+  return GetElementPtrInst::Create(GEP.getResultElementType(), NewPtr,

IRBuilder needs to be used for all but the last instruction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155688

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

[PATCH] D155688: [PATCH] [llvm] [InstCombine] Canonicalise ADD+GEP

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2338-2340
+if (BinaryOperator *Idx =
+dyn_cast_or_null(GEP.getOperand(1)))
+  if ((Idx->getOpcode() == Instruction::Add) && Idx->hasOneUse()) {





Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2346
+//   %newgep = getelementptr i32, i32* %newptr, i64 %idx2
+Value *Ptr = GEP.getOperand(0);
+auto *NewPtr = GetElementPtrInst::Create(





Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2349
+GEP.getResultElementType(), Ptr, Idx->getOperand(0), "", &GEP);
+auto *NewGEP = GetElementPtrInst::Create(GEP.getResultElementType(),
+ NewPtr, Idx->getOperand(1));

No need for the NewGEP variable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155688

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

[PATCH] D155688: [PATCH] [llvm] [InstCombine] Reassociate loop invariant GEP index calculations.

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2336
+// Try to reassociate loop invariant index calculations to enable LICM.
+if (Idx && (Idx->getOpcode() == Instruction::Add)) {
+  Value *Ptr = GEP.getOperand(0);

This needs a one-use check. The transform is not profitable if we have to keep 
*both* the add and the gep.

Can also use `match(GEP.getOperand(1), m_Add(...))` here.



Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2339
+  Value *InvIdx = Idx->getOperand(0);
+  Value *NonInvIdx = Idx->getOperand(1);
+

This no longer checks for loop invariance, so we should remove any 
invariance-related terminology.



Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:2351
+   NewPtr, NonInvIdx);
+  NewGEP->setIsInBounds(GEP.isInBounds());
+  return NewGEP;

This inbounds preservation is incorrect: https://alive2.llvm.org/ce/z/bJZvQG

It's even incorrect if the add is also nsw.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155688

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D86310#4648646 , @hvdijk wrote:

> In D86310#4648634 , @nikic wrote:
>
>> I'm happy to sign off on the x86-64 part here, but I'm less sure about 
>> x86-32. If I understood correctly, the i128 alignment is raised there 
>> exclusively to fix the "f128 legalized to i128 libcall" case -- is there any 
>> other ABI requirement for i128 alignment on x86-32? Is raising i128 
>> alignment the right way to fix an f128 issue?
>
> GCC does not support `__int128` on x86-32, but clang has the 
> `-fforce-enable-int128` option, and when that is used, it gives it the same 
> 16-byte alignment that it does on x86-64, so even ignoring the `_Float128` 
> issue, I think the change is right for x86-32.

Okay, that's a compelling argument.




Comment at: llvm/lib/IR/AutoUpgrade.cpp:5233
+  SmallVector Groups;
+  Regex R("(.*-i64:64)(-.*)");
+  if (R.match(Res, &Groups))

I don't think this will work for the 32-bit targets that don't have `-i64:64`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

I'm happy to sign off on the x86-64 part here, but I'm less sure about x86-32. 
If I understood correctly, the i128 alignment is raised there exclusively to 
fix the "f128 legalized to i128 libcall" case -- is there any other ABI 
requirement for i128 alignment on x86-32? Is raising i128 alignment the right 
way to fix an f128 issue?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86310

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

[PATCH] D158169: [X86] Fix i128 argument passing under SysV ABI

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D158169#4645012 , @RalfJung wrote:

>> I think the CCIfSplit means it was larger than i64 to start.
>
> What about the case where `R9` would take the *second* half of a split i128 
> (i.e., the value gets split across `R8` and `R9`)? Does that still work after 
> this patch?

Yes, this is the case tested in `@in_reg`. `CCIfSplit` only matches the first 
part of a split value, not all parts of it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158169

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

[PATCH] D158984: [Clang] Allow __declspec(noalias) to access inaccessible memory

[Nikita Popov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG14cc7a077275: [Clang] Allow __declspec(noalias) to access 
inaccessible memory (authored by nikic).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158984

Files:
  clang/lib/CodeGen/CGCall.cpp
  clang/test/CodeGen/ms-declspecs.c


Index: clang/test/CodeGen/ms-declspecs.c
===
--- clang/test/CodeGen/ms-declspecs.c
+++ clang/test/CodeGen/ms-declspecs.c
@@ -41,4 +41,4 @@
 // CHECK: attributes [[NUW]] = { nounwind{{.*}} }
 // CHECK: attributes [[NI]] = { noinline nounwind{{.*}} }
 // CHECK: attributes [[NR]] = { noreturn }
-// CHECK: attributes [[NA]] = { nounwind memory(argmem: readwrite){{.*}} }
+// CHECK: attributes [[NA]] = { nounwind memory(argmem: readwrite, 
inaccessiblemem: readwrite){{.*}} }
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2375,7 +2375,7 @@
   // gcc specifies that 'pure' functions cannot have infinite loops.
   FuncAttrs.addAttribute(llvm::Attribute::WillReturn);
 } else if (TargetDecl->hasAttr()) {
-  FuncAttrs.addMemoryAttr(llvm::MemoryEffects::argMemOnly());
+  FuncAttrs.addMemoryAttr(llvm::MemoryEffects::inaccessibleOrArgMemOnly());
   FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
 }
 if (TargetDecl->hasAttr())


Index: clang/test/CodeGen/ms-declspecs.c
===
--- clang/test/CodeGen/ms-declspecs.c
+++ clang/test/CodeGen/ms-declspecs.c
@@ -41,4 +41,4 @@
 // CHECK: attributes [[NUW]] = { nounwind{{.*}} }
 // CHECK: attributes [[NI]] = { noinline nounwind{{.*}} }
 // CHECK: attributes [[NR]] = { noreturn }
-// CHECK: attributes [[NA]] = { nounwind memory(argmem: readwrite){{.*}} }
+// CHECK: attributes [[NA]] = { nounwind memory(argmem: readwrite, inaccessiblemem: readwrite){{.*}} }
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -2375,7 +2375,7 @@
   // gcc specifies that 'pure' functions cannot have infinite loops.
   FuncAttrs.addAttribute(llvm::Attribute::WillReturn);
 } else if (TargetDecl->hasAttr()) {
-  FuncAttrs.addMemoryAttr(llvm::MemoryEffects::argMemOnly());
+  FuncAttrs.addMemoryAttr(llvm::MemoryEffects::inaccessibleOrArgMemOnly());
   FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
 }
 if (TargetDecl->hasAttr())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D158972: [DebugInfo] Fix incorrect dbg.declare when nrvo flag is used

[Nikita Popov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG13a044c6993a: [DebugInfo] Fix incorrect dbg.declare when 
nrvo flag is used (authored by nikic).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158972

Files:
  clang/lib/CodeGen/CGDecl.cpp
  clang/test/CodeGenCXX/trivial_abi_debuginfo.cpp


Index: clang/test/CodeGenCXX/trivial_abi_debuginfo.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/trivial_abi_debuginfo.cpp
@@ -0,0 +1,33 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py 
UTC_ARGS: --functions "makeTrivial" --version 2
+// RUN: %clang_cc1 -debug-info-kind=limited -triple x86_64-unknown-linux-gnu 
-emit-llvm -o - %s | FileCheck %s
+
+struct __attribute__((trivial_abi)) Trivial {
+  ~Trivial() {}
+  int ivar = 10;
+};
+
+// The dbg.declare should be on %retval, not on %nrvo.
+
+// CHECK-LABEL: define dso_local i32 @_Z11makeTrivialv
+// CHECK-SAME: () #[[ATTR0:[0-9]+]] !dbg [[DBG5:![0-9]+]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[RETVAL:%.*]] = alloca [[STRUCT_TRIVIAL:%.*]], align 4
+// CHECK-NEXT:[[NRVO:%.*]] = alloca i1, align 1
+// CHECK-NEXT:store i1 false, ptr [[NRVO]], align 1, !dbg [[DBG18:![0-9]+]]
+// CHECK-NEXT:call void @llvm.dbg.declare(metadata ptr [[RETVAL]], 
metadata [[META19:![0-9]+]], metadata !DIExpression()), !dbg [[DBG20:![0-9]+]]
+// CHECK-NEXT:call void @_ZN7TrivialC1Ev(ptr noundef nonnull align 4 
dereferenceable(4) [[RETVAL]]) #[[ATTR3:[0-9]+]], !dbg [[DBG20]]
+// CHECK-NEXT:store i1 true, ptr [[NRVO]], align 1, !dbg [[DBG21:![0-9]+]]
+// CHECK-NEXT:[[NRVO_VAL:%.*]] = load i1, ptr [[NRVO]], align 1, !dbg 
[[DBG22:![0-9]+]]
+// CHECK-NEXT:br i1 [[NRVO_VAL]], label [[NRVO_SKIPDTOR:%.*]], label 
[[NRVO_UNUSED:%.*]], !dbg [[DBG22]]
+// CHECK:   nrvo.unused:
+// CHECK-NEXT:call void @_ZN7TrivialD1Ev(ptr noundef nonnull align 4 
dereferenceable(4) [[RETVAL]]) #[[ATTR3]], !dbg [[DBG22]]
+// CHECK-NEXT:br label [[NRVO_SKIPDTOR]], !dbg [[DBG22]]
+// CHECK:   nrvo.skipdtor:
+// CHECK-NEXT:[[COERCE_DIVE:%.*]] = getelementptr inbounds 
[[STRUCT_TRIVIAL]], ptr [[RETVAL]], i32 0, i32 0, !dbg [[DBG22]]
+// CHECK-NEXT:[[TMP0:%.*]] = load i32, ptr [[COERCE_DIVE]], align 4, !dbg 
[[DBG22]]
+// CHECK-NEXT:ret i32 [[TMP0]], !dbg [[DBG22]]
+//
+Trivial makeTrivial() {
+  Trivial ret_val;
+  return ret_val;
+}
Index: clang/lib/CodeGen/CGDecl.cpp
===
--- clang/lib/CodeGen/CGDecl.cpp
+++ clang/lib/CodeGen/CGDecl.cpp
@@ -1534,8 +1534,7 @@
   // applied.
   llvm::Value *Zero = Builder.getFalse();
   Address NRVOFlag =
-  CreateTempAlloca(Zero->getType(), CharUnits::One(), "nrvo",
-   /*ArraySize=*/nullptr, &AllocaAddr);
+  CreateTempAlloca(Zero->getType(), CharUnits::One(), "nrvo");
   EnsureInsertPoint();
   Builder.CreateStore(Zero, NRVOFlag);
 


Index: clang/test/CodeGenCXX/trivial_abi_debuginfo.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/trivial_abi_debuginfo.cpp
@@ -0,0 +1,33 @@
+// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --functions "makeTrivial" --version 2
+// RUN: %clang_cc1 -debug-info-kind=limited -triple x86_64-unknown-linux-gnu -emit-llvm -o - %s | FileCheck %s
+
+struct __attribute__((trivial_abi)) Trivial {
+  ~Trivial() {}
+  int ivar = 10;
+};
+
+// The dbg.declare should be on %retval, not on %nrvo.
+
+// CHECK-LABEL: define dso_local i32 @_Z11makeTrivialv
+// CHECK-SAME: () #[[ATTR0:[0-9]+]] !dbg [[DBG5:![0-9]+]] {
+// CHECK-NEXT:  entry:
+// CHECK-NEXT:[[RETVAL:%.*]] = alloca [[STRUCT_TRIVIAL:%.*]], align 4
+// CHECK-NEXT:[[NRVO:%.*]] = alloca i1, align 1
+// CHECK-NEXT:store i1 false, ptr [[NRVO]], align 1, !dbg [[DBG18:![0-9]+]]
+// CHECK-NEXT:call void @llvm.dbg.declare(metadata ptr [[RETVAL]], metadata [[META19:![0-9]+]], metadata !DIExpression()), !dbg [[DBG20:![0-9]+]]
+// CHECK-NEXT:call void @_ZN7TrivialC1Ev(ptr noundef nonnull align 4 dereferenceable(4) [[RETVAL]]) #[[ATTR3:[0-9]+]], !dbg [[DBG20]]
+// CHECK-NEXT:store i1 true, ptr [[NRVO]], align 1, !dbg [[DBG21:![0-9]+]]
+// CHECK-NEXT:[[NRVO_VAL:%.*]] = load i1, ptr [[NRVO]], align 1, !dbg [[DBG22:![0-9]+]]
+// CHECK-NEXT:br i1 [[NRVO_VAL]], label [[NRVO_SKIPDTOR:%.*]], label [[NRVO_UNUSED:%.*]], !dbg [[DBG22]]
+// CHECK:   nrvo.unused:
+// CHECK-NEXT:call void @_ZN7TrivialD1Ev(ptr noundef nonnull align 4 dereferenceable(4) [[RETVAL]]) #[[ATTR3]], !dbg [[DBG22]]
+// CHECK-NEXT:br label [[NRVO_SKIPDTOR]], !dbg [[DBG22]]
+// CHECK:   nrvo.skipdtor:
+// CHECK-NEX

[PATCH] D112921: [clang] Enable sized deallocation by default in C++14 onwards

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

FYI this resulted in some pretty wild code size swings, in particular between 
-10% and -15% for tramp3d-v4 
(http://llvm-compile-time-tracker.com/compare.php?from=6cde64a94986165547ae5237ac7dd4bddfc9f2a7&to=2916b125f686115deab2ba573dcaff3847566ab9&stat=size-text).
 Not sure whether that's an expected result of this change or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112921

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

[PATCH] D157497: feat: Migrate isArch16Bit

[Nikita Popov via Phabricator via cfe-commits]

nikic requested changes to this revision.
nikic added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: StephenFan.

This change looks strictly worse in isolation, the proposal on discourse did 
not reach consensus, and looking at the diagram in 
https://discourse.llvm.org/t/rfc-refactor-triple-related-classes/70410/11 there 
is zero chance that it is ever going to be accepted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157497

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

[PATCH] D158169: [X86] Fix i128 argument passing under SysV ABI

[Nikita Popov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGfa1b6e6b34eb: [X86] Fix i128 argument passing under SysV ABI 
(authored by nikic).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D158169?vs=551075&id=551946#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158169

Files:
  clang/docs/ReleaseNotes.rst
  llvm/lib/Target/X86/X86CallingConv.td
  llvm/test/CodeGen/X86/addcarry.ll
  llvm/test/CodeGen/X86/i128-abi.ll
  llvm/test/CodeGen/X86/sadd_sat_vec.ll
  llvm/test/CodeGen/X86/ssub_sat_vec.ll
  llvm/test/CodeGen/X86/subcarry.ll
  llvm/test/CodeGen/X86/uadd_sat_vec.ll
  llvm/test/CodeGen/X86/usub_sat_vec.ll

Index: llvm/test/CodeGen/X86/usub_sat_vec.ll
===
--- llvm/test/CodeGen/X86/usub_sat_vec.ll
+++ llvm/test/CodeGen/X86/usub_sat_vec.ll
@@ -1057,10 +1057,10 @@
 ; SSE:   # %bb.0:
 ; SSE-NEXT:movq %rdi, %rax
 ; SSE-NEXT:xorl %edi, %edi
-; SSE-NEXT:subq %r9, %rsi
+; SSE-NEXT:subq {{[0-9]+}}(%rsp), %rsi
 ; SSE-NEXT:sbbq {{[0-9]+}}(%rsp), %rdx
-; SSE-NEXT:cmovbq %rdi, %rsi
 ; SSE-NEXT:cmovbq %rdi, %rdx
+; SSE-NEXT:cmovbq %rdi, %rsi
 ; SSE-NEXT:subq {{[0-9]+}}(%rsp), %rcx
 ; SSE-NEXT:sbbq {{[0-9]+}}(%rsp), %r8
 ; SSE-NEXT:cmovbq %rdi, %r8
@@ -1075,10 +1075,10 @@
 ; AVX:   # %bb.0:
 ; AVX-NEXT:movq %rdi, %rax
 ; AVX-NEXT:xorl %edi, %edi
-; AVX-NEXT:subq %r9, %rsi
+; AVX-NEXT:subq {{[0-9]+}}(%rsp), %rsi
 ; AVX-NEXT:sbbq {{[0-9]+}}(%rsp), %rdx
-; AVX-NEXT:cmovbq %rdi, %rsi
 ; AVX-NEXT:cmovbq %rdi, %rdx
+; AVX-NEXT:cmovbq %rdi, %rsi
 ; AVX-NEXT:subq {{[0-9]+}}(%rsp), %rcx
 ; AVX-NEXT:sbbq {{[0-9]+}}(%rsp), %r8
 ; AVX-NEXT:cmovbq %rdi, %r8
Index: llvm/test/CodeGen/X86/uadd_sat_vec.ll
===
--- llvm/test/CodeGen/X86/uadd_sat_vec.ll
+++ llvm/test/CodeGen/X86/uadd_sat_vec.ll
@@ -1161,11 +1161,11 @@
 ; SSE-LABEL: v2i128:
 ; SSE:   # %bb.0:
 ; SSE-NEXT:movq %rdi, %rax
-; SSE-NEXT:addq %r9, %rsi
+; SSE-NEXT:addq {{[0-9]+}}(%rsp), %rsi
 ; SSE-NEXT:adcq {{[0-9]+}}(%rsp), %rdx
 ; SSE-NEXT:movq $-1, %rdi
-; SSE-NEXT:cmovbq %rdi, %rsi
 ; SSE-NEXT:cmovbq %rdi, %rdx
+; SSE-NEXT:cmovbq %rdi, %rsi
 ; SSE-NEXT:addq {{[0-9]+}}(%rsp), %rcx
 ; SSE-NEXT:adcq {{[0-9]+}}(%rsp), %r8
 ; SSE-NEXT:cmovbq %rdi, %r8
@@ -1179,11 +1179,11 @@
 ; AVX-LABEL: v2i128:
 ; AVX:   # %bb.0:
 ; AVX-NEXT:movq %rdi, %rax
-; AVX-NEXT:addq %r9, %rsi
+; AVX-NEXT:addq {{[0-9]+}}(%rsp), %rsi
 ; AVX-NEXT:adcq {{[0-9]+}}(%rsp), %rdx
 ; AVX-NEXT:movq $-1, %rdi
-; AVX-NEXT:cmovbq %rdi, %rsi
 ; AVX-NEXT:cmovbq %rdi, %rdx
+; AVX-NEXT:cmovbq %rdi, %rsi
 ; AVX-NEXT:addq {{[0-9]+}}(%rsp), %rcx
 ; AVX-NEXT:adcq {{[0-9]+}}(%rsp), %r8
 ; AVX-NEXT:cmovbq %rdi, %r8
Index: llvm/test/CodeGen/X86/subcarry.ll
===
--- llvm/test/CodeGen/X86/subcarry.ll
+++ llvm/test/CodeGen/X86/subcarry.ll
@@ -21,7 +21,7 @@
 ; CHECK-LABEL: sub256:
 ; CHECK:   # %bb.0: # %entry
 ; CHECK-NEXT:movq %rdi, %rax
-; CHECK-NEXT:subq %r9, %rsi
+; CHECK-NEXT:subq {{[0-9]+}}(%rsp), %rsi
 ; CHECK-NEXT:sbbq {{[0-9]+}}(%rsp), %rdx
 ; CHECK-NEXT:sbbq {{[0-9]+}}(%rsp), %rcx
 ; CHECK-NEXT:sbbq {{[0-9]+}}(%rsp), %r8
Index: llvm/test/CodeGen/X86/ssub_sat_vec.ll
===
--- llvm/test/CodeGen/X86/ssub_sat_vec.ll
+++ llvm/test/CodeGen/X86/ssub_sat_vec.ll
@@ -2026,27 +2026,27 @@
 ; SSE-NEXT:subq {{[0-9]+}}(%rsp), %rcx
 ; SSE-NEXT:sbbq {{[0-9]+}}(%rsp), %r8
 ; SSE-NEXT:seto %dil
-; SSE-NEXT:movq %r8, %r10
-; SSE-NEXT:sarq $63, %r10
+; SSE-NEXT:movq %r8, %r9
+; SSE-NEXT:sarq $63, %r9
 ; SSE-NEXT:testb %dil, %dil
-; SSE-NEXT:cmovneq %r10, %rcx
-; SSE-NEXT:movabsq $-9223372036854775808, %r11 # imm = 0x8000
-; SSE-NEXT:xorq %r11, %r10
+; SSE-NEXT:cmovneq %r9, %rcx
+; SSE-NEXT:movabsq $-9223372036854775808, %r10 # imm = 0x8000
+; SSE-NEXT:xorq %r10, %r9
 ; SSE-NEXT:testb %dil, %dil
-; SSE-NEXT:cmoveq %r8, %r10
-; SSE-NEXT:subq %r9, %rsi
+; SSE-NEXT:cmoveq %r8, %r9
+; SSE-NEXT:subq {{[0-9]+}}(%rsp), %rsi
 ; SSE-NEXT:sbbq {{[0-9]+}}(%rsp), %rdx
 ; SSE-NEXT:seto %dil
 ; SSE-NEXT:movq %rdx, %r8
 ; SSE-NEXT:sarq $63, %r8
 ; SSE-NEXT:testb %dil, %dil
 ; SSE-NEXT:cmovneq %r8, %rsi
-; SSE-NEXT:xorq %r11, %r8
+; SSE-NEXT:xorq %r10, %r8
 ; SSE-NEXT:testb %dil, %dil
 ; SSE-NEXT:cmoveq %rdx, %r8
 ; SSE-NEXT:movq %rcx, 16(%rax)
 ; SSE-NEXT:movq %rsi, (%rax)
-; SSE-NEXT:movq %r10, 24(%rax)
+; SSE-NEXT:movq %r9, 24(%rax)
 ; SSE

[PATCH] D156571: [DebugInfo] Alternate MD5 fix, NFC

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

Thanks! Can confirm that this recovers the compile-time regression.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156571

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

[PATCH] D157151: [Driver] Refactor to use llvm Option's new Visibility flags

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

To avoid a full revert, I removed the deprecations in this patch in 
https://github.com/llvm/llvm-project/commit/348d36f1a2c8c776ce87e038bf15fc4cabd62702.
 Please do not deprecate methods that have remaining in-tree users and make 
sure that `-Werror=deprecated-declarations` builds work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157151

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

Please split the warning fixes off into a separate patch.


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

https://reviews.llvm.org/D152495

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

[PATCH] D86993: Document Clang's expectations of the C standard library.

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

@aaron.ballman I wanted to check back whether the linked document is what you 
had in mind, or whether some more/else would be needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86993

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

[PATCH] D155773: [llvm][MemoryBuiltins] Add alloca support to getInitialValueOfAllocation

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

@jmciver Thanks for the context. I would recommend you to put up the patch you 
have based on top of this, because it's pretty hard to tell whether this API 
design makes sense without seeing the use. I have some doubts about that, for 
two reasons:

- If I understand correctly, for your use case getInitialValueOfAllocation() 
can't just return a constant anymore, it may have to insert an instruction. 
Additionally, there is no longer a single "initial value", but it may wary 
between different loads from the same allocation. This means that the API is 
going to change to the point that getInitialValueOfAllocation() is no longer 
recognizable (and probably no longer correctly named -- it would be more 
materializeInitialValueOfAllocation()).
- As some of the changes in this patch show, we also have to give 
lifetime.start intrinsics similar treatment to allocas, but this doesn't quite 
fit the current API.

I think supporting allocas in getInitialValueOfAllocation() is perfectly fine, 
but I'm not sure this really brings you closer to what you want to do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155773

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

[PATCH] D157227: [Clang] Don't add `undef` for `operator new` under -fno-exceptions.

[Nikita Popov via Phabricator via cfe-commits]

nikic requested changes to this revision.
nikic added a comment.
This revision now requires changes to proceed.

Removing noundef makes no sense to me, because the return value is noundef even 
under fno-exceptions. If we remove something, it should be the nonnull 
attribute, because that's what's actually being violated.

@ldionne Can you give us an ETA on D150610  
please? Do we need to introduce a workaround for this libcxx bug in clang, or 
do you plan to fix this in the near future?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157227

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

[PATCH] D105671: [Intrinsics][ObjC] Mark objc_retain and friends as thisreturn.

[Nikita Popov via Phabricator via cfe-commits]

nikic requested changes to this revision.
nikic added a comment.
This revision now requires changes to proceed.
Herald added a subscriber: StephenFan.

Please separate the change to stripPointerCasts() into a separate review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105671

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

[PATCH] D155997: [Phase Ordering] Don't speculate in SimplifyCFG before PGO annotation

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

I'm not a fan of the PGO conditional behavior here. I'd prefer to disable 
speculation unconditionally if that is feasible. This is basically what D153392 
 did. While the specific test case there was 
addressed in a different way, if we take it in conjunction with this one, I 
think we have enough motivation to do the change.

Another piece of motivation would be that the EarlyFPM change would allow us to 
move LowerExpectIntrinsic to the end of EarlyFPM again, which would make expect 
intrinsics work in Rust again. The pass was moved earlier to make the first 
SimplifyCFG run take them into account, but that would no longer be a problem 
if we disable speculation.

And another piece of motivation would be that we could move SimplifyCFG after 
SROA, where it can be much more effective (but doing so with speculation breaks 
IPSCCP too much). I think doing that needs some further changes, but it's a 
move in the right direction.

So I think the EarlyFPM change at least seems like something we should pretty 
clearly do independently of PGO. The addPGOInstrPasses() change is of course 
PGO specific anyway. I think only the GlobalCleanupPM change may not be 
completely clear cut.




Comment at: llvm/test/Transforms/PhaseOrdering/simplifycfg-speculate-blocks.ll:1
+;; Check that SimplifyCFG does not attempt speculation until after PGO is
+;; annotated in the IR, and then does not perform it when unprofitable.

aeubanks wrote:
> hmm typically these phase ordering tests use 
> `llvm/utils/update_test_checks.py`, but that doesn't support the 
> llvm-profdata RUN line. I think a non-update_test_checks test is probably 
> fine for this, @nikic does that make sense?
> 
> 
I thought UTC supports unrecognized commands (by just executing them)... If 
not, a non-UTC test is fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155997

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

[PATCH] D155991: [DWARF] Make sure file entry for artificial functions has an MD5 checksum

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

FYI this is a 0.5% compile-time regression on `O0` builds 
(https://llvm-compile-time-tracker.com/compare.php?from=69593aa5c054cec6be6b822c073ccdc63748a68d&to=7abb5fc618cec66841a8280d2a099a4c9c8cb91b&stat=instructions:u).
 Is that expected?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155991

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

[PATCH] D86993: Document Clang's expectations of the C standard library.

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

Sorry for the delay, this took more time than expected. I've created an initial 
draft here: 
https://docs.google.com/document/d/1guH_HgibKrX7t9JfKGfWX2UCPyZOTLsnRfR6UleD1F8/edit?usp=sharing

While writing this I remembered another bit that is somewhat relevant here: 
Clang explicitly ignores the nonnull attributes on declarations in order to 
avoid miscompilations for uses of these libc functions. There are sanitizer 
checks for them, but Clang will not optimize based on nonnull assumptions, even 
if a libc implementation is used that adds the attributes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86993

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

[PATCH] D155773: [llvm][MemoryBuiltins] Add alloca support to getInitialValueOfAllocation

[Nikita Popov via Phabricator via cfe-commits]

nikic requested changes to this revision.
nikic added a comment.
This revision now requires changes to proceed.

> This commit is in support of future uninitialized memory handling and adds
> alloca instruction support to getInitialValueOfAllocation. This unifies 
> initial
> memory state querying (both stack and heap) to a single utility function.

Could you explain in more detail what future work you have based on this? Is 
this about the promotion of malloc (and similar) memory?

In any case, I don't think it's acceptable to go through TLI as a required 
dependency for mem2reg. Alloca promotion doesn't need it, and that's what all 
the existing code is doing. This should at least be an optional dependency.

However, I think it would be better to directly pass the initial value to 
mem2reg instead (defaulting to UndefValue), rather than making it query it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155773

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

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D86310#4495825 , @hvdijk wrote:

> A thought occurs: in older versions of LLVM, the data layout mechanism worked 
> differently and permitted targets to declare that they supported multiple 
> different data layout strings, by overriding `isCompatibleDataLayout`. This 
> mechanism was removed in D67631 . If we 
> reinstate that, we can have the X86 target declare that it "supports" data 
> layout strings with and without the `-i128:128`, where by "supports", I mean 
> the code continues to not generally work in the same way it does not 
> generally work now, but the specific limited cases that do work continue to 
> work exactly the same ABI-incompatible way. This would have the same result 
> of bug-for-bug compatibility with existing modules, but in what I suspect 
> would be a significantly simpler way than by going through the module and 
> adding explicit alignments everywhere. While I would still prefer to give up 
> on that compatibility, if it is a hard requirement, and if this would be an 
> alternative way of achieving it, I might possibly be able to update this 
> patch to do just that. Would this be acceptable?

The main problem with that is that we can't have multiple data layouts for one 
module, so linking old and new bitcode together would fail. But maybe that's 
exactly what we want -- after all, it is incompatible. Even if we "correctly" 
upgraded to preserve behavior of the old bitcode, it would still be 
incompatible with the new bitcode if i128 crosses the ABI boundary (explicitly 
or implicitly).


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

https://reviews.llvm.org/D86310

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

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D145265#4478621 , @aeubanks wrote:

> @nikic could you try running this over the rust tests again?

Ignoring some unrelated powerpc data layout failures, these codegen tests fail:

  [codegen] tests/codegen/issues/issue-45222.rs
  [codegen] tests/codegen/issues/issue-45466.rs
  [codegen] tests/codegen/issues/issue-69101-bounds-check.rs
  [codegen] tests/codegen/iter-repeat-n-trivial-drop.rs
  [codegen] tests/codegen/slice-as_chunks.rs

The `iter-repeat-n-trivial-drop.rs` and `slice-as_chunks.rs` failures are 
cosmetic.

`issues-45222.rs` is the same as last time (loop not folded to constant).

`issue-45466.rs` fails to convert a loop into a memset 0 (this looks like the 
simplest test case, because it has essentially just one function in initial IR).

`issue-69101-bounds-check.rs` leaves behind panic_bounds_checks in 
already_sliced_no_bounds_check.

I've put the input IR for these three failure up at 
https://gist.github.com/nikic/9ec0bc02d451e7abd0f8d78800c2206c.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145265

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

[PATCH] D86993: Document Clang's expectations of the C standard library.

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D86993#4477080 , @aaron.ballman 
wrote:

> In D86993#4474682 , @nikic wrote:
>
>>> I continue to think it's a mistake for us to document that Clang will not 
>>> work with a conforming C standard library implementation unless we're 
>>> filing issues with WG14 to alert them to the reasons why. If there's a DR 
>>> filed with the committee, that gives us something to point our users to as 
>>> justification. Otherwise, our users will correctly put these bugs at our 
>>> feet.
>>
>> As you are involved with WG14, can you please take care of that? I couldn't 
>> even find out how one is supposed to file a DR.
>
> I'm happy to help with it, but I'll need some outside help as well so I get 
> the technical details and justifications correct and can defend them when 
> presenting to the committee. If someone could put together a super rough 
> draft of what changes we'd like to see and why, I can definitely work with 
> them to polish it into a final document, get it to the committee, and 
> champion it there. Related question though: what do we do if the committee 
> rejects the DR? I think LLVM and Clang have slightly different pressures 
> here, so we might have different answers.
>
> FWIW, you can find more info here about how to communicate with WG14: 
> https://www.open-std.org/jtc1/sc22/wg14/www/contributing

Thank you! I will prepare an initial draft and share it here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86993

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

[PATCH] D86993: Document Clang's expectations of the C standard library.

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D86993#4474392 , @rjmccall wrote:

> In D86993#4474267 , @RalfJung wrote:
>
>> The first point is important for LLVM's own memcpy/memmove intrinsics, which 
>> are documented as NOPs on size 0 (and e.g. Rust relies on that).
>
> Right, I understand that these assumptions come directly from the stronger 
> semantics offered by the LLVM intrinsics.  The C committee is not going to 
> find that compelling, though — we don't get to default-win arguments just 
> because we've defined an IR with stronger semantics than necessary.  They are 
> going to want to see arguments about why it's valuable for the C library to 
> have these stronger semantics, which for us means talking about code patterns 
> in user programs that take advantage of those stronger semantics and the 
> benefits they see from that.

I think the argument for this one is pretty clear:

- Lower code size: No need to add an explicit `n != 0` check.
- Better performance: If the `n == 0` case is rare, you don't have to do that 
redundant check.
- Better security: You are less likely to cause UB by forgetting a `n != 0` 
check in place that needs it.

When I made an old code base ubsan compliant some time ago, adding these checks 
certainly felt like the one case where fixing the sanitizer diagnostics made 
the code objectively worse on all possible metrics. Take into account that the 
requirement for nonnull pointers for `n != 0` does not allow any useful 
optimization of the memcpy implementation, and this seems like a no-brainer.

In D86993#4474549 , @RalfJung wrote:

> I was told that "glibc explicitly only marks memcpy arguments as nonnull for 
> external use of headers (i.e. the nonnull assumptions are disabled when 
> compiling the implementation)"... okay now re-reading this, that's a 
> statement about glibc, not GCC.

Right. glibc only adds nonnull annotations to declarations when included by 
users of glibc. The implementation is compiled without any such assumptions 
(though of course, usually you get an assembly implementation anyway). Musl 
doesn't annotate nonnull either way.

> Anyway this is outside my expertise. I will try to add Nikita here who said 
> "GCC also requires passing equal pointer to memcpy to be legal".

GCC uses equal pointer memcpy in pretty much exactly the same cases as Clang 
does: Aggregate assignments.

> I continue to think it's a mistake for us to document that Clang will not 
> work with a conforming C standard library implementation unless we're filing 
> issues with WG14 to alert them to the reasons why. If there's a DR filed with 
> the committee, that gives us something to point our users to as 
> justification. Otherwise, our users will correctly put these bugs at our feet.

As you are involved with WG14, can you please take care of that? I couldn't 
even find out how one is supposed to file a DR.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86993

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

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

(The patch description could use an update...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148216

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

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: StephenFan.

LGTM

> Fixed. Does this test case exist somewhere? I couldn't find it upstream.

That wasn't an existing test case. Though if you add a `--check-globals all` 
variant to generated-funcs.c, I believe that would cover that case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148216

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

[PATCH] D154285: [clang] Remove CGBuilderTy::CreateElementBitCast

[Nikita Popov via Phabricator via cfe-commits]

nikic accepted this revision.
nikic 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/D154285/new/

https://reviews.llvm.org/D154285

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

[PATCH] D154285: [clang] Remove CGBuilderTy::CreateElementBitCast

[Nikita Popov via Phabricator via cfe-commits]

nikic requested changes to this revision.
nikic added a comment.
This revision now requires changes to proceed.

The entire change is NFC cleanup, it makes no sense to separate it. Please 
revert to previous patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154285

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

[PATCH] D154285: [clang] Deprecate CGBuilderTy::CreateElementBitCast

[Nikita Popov via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/lib/CodeGen/CGBuilder.h:158
 
   /// This method is to be deprecated. Use `Address::withElementType` instead.
+  [[deprecated("Use `Address::withElementType` instead.")]]

JOE1994 wrote:
> 
This is a private method, so simply delete it instead of deprecating.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154285

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

[PATCH] D153314: [clang] Replace uses of CGBuilderTy::CreateElementBitCast (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic accepted this revision.
nikic 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/D153314/new/

https://reviews.llvm.org/D153314

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

[PATCH] D153314: [clang] Replace uses of CGBuilderTy::CreateElementBitCast (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: clang/lib/CodeGen/Targets/AArch64.cpp:522-523
 auto *Load = CGF.Builder.CreateLoad(VAListAddr);
 Address Addr = Address(Load, CGF.Int8Ty, SlotSize);
-return CGF.Builder.CreateElementBitCast(Addr, CGF.ConvertTypeForMem(Ty));
+return Addr.withElementType(CGF.ConvertTypeForMem(Ty));
   }




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153314

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

[PATCH] D153728: [llvm] Move AttributeMask to a separate header

[Nikita Popov via Phabricator via cfe-commits]

nikic accepted this revision.
nikic 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/D153728/new/

https://reviews.llvm.org/D153728

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

[PATCH] D153694: [clang][CodeGen] Remove no-op EmitCastToVoidPtr (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:1473
   QualType SrcRecordTy,
   QualType DestTy) {
   auto *ClassDecl =

barannikov88 wrote:
> `DestTy` has become unused in both implementations. I'm not sure if I should 
> remove it. It is always a [cv-qualified] `void *`.
> 
Removing it sounds reasonable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153694

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

[PATCH] D144970: [llvm-c] Remove bindings for creating legacy passes

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D144970#4434305 , @yamt wrote:

> at least some of these stuff is used by wasm-micro-runtime. do you have a 
> suggestion about how to adapt it?
> https://github.com/bytecodealliance/wasm-micro-runtime/blob/72fc872afe9a51228b2a32bc7b08475b176f187f/core/iwasm/compilation/aot_compiler.c#L2666-L2670

You can use `LLVMRunPasses` with `lower-atomic,lower-switch` or so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144970

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

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

There are some test failures.

I believe there is one bug with the handling of unnamed globals. Previously we 
produced this:

  ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py 
UTC_ARGS: --check-globals --version 2
  ; RUN: opt -S < %s | FileCheck %s
  @0 = global i32 0
  
  ;.
  ; CHECK: @[[GLOB0:[0-9]+]] = global i32 0
  ;.
  define i32 @test() {
  ; CHECK-LABEL: define i32 @test() {
  ; CHECK-NEXT:[[V:%.*]] = load i32, ptr @[[GLOB0]], align 4
  ; CHECK-NEXT:ret i32 [[V]]
  ;
%v = load i32, ptr @0
ret i32 %v
  }

And now we instead check `CHECK: @0 = global i32 0`, so there is a mismatch 
between definition and use. I believe it does make sense to keep the wildcard 
names for unnamed globals.

On the same test case, if I keep rerunning update_test_checks on the same file, 
it keeps adding extra `CHECK: @0 = global i32 0` lines at the start (before the 
first `;.`. That didn't happen previously.




Comment at: clang/test/utils/update_cc_test_checks/Inputs/annotations.c:1
+// UTC_ARGS: --version 3
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -fblocks 
-ftrivial-auto-var-init=zero %s -emit-llvm -o - | FileCheck %s

It would be more typical to pass `--version 3` when calling 
update_cc_test_checks. (Note that this is only a quirk of the test setup. 
Outside tests the `--version 3` is implied for new tests.)



Comment at: 
llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/check_attrs.ll.funcattrs.expected:9
 define ptr @foo(ptr %s) nounwind uwtable readnone optsize ssp {
-; CHECK: Function Attrs: mustprogress nofree norecurse nosync nounwind optsize 
ssp willreturn memory(none) uwtable
+; CHECK: Function Attrs: nofree norecurse nosync nounwind optsize ssp 
willreturn memory(none) uwtable
 ; CHECK-LABEL: define {{[^@]+}}@foo

Huh, where does this change come from?



Comment at: llvm/utils/UpdateTestChecks/common.py:991
+NamelessValue(r"META"   , "!" , r"![a-z.]+ "   , r"![0-9]+"
 , None ) ,
+]
+

Does this still comply with the formatter?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148216

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

[PATCH] D153314: [clang] Replace uses of CGBuilderTy::CreateElementBitCast (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: clang/lib/CodeGen/CGNonTrivialStruct.cpp:370
 llvm::Value *DstArrayEnd =
 CGF.Builder.CreateInBoundsGEP(CGF.Int8Ty, BC.getPointer(), 
SizeInBytes);
 DstArrayEnd = CGF.Builder.CreateBitCast(

Only `BC.getPointer()` is used, so you can omit the withElementType.



Comment at: clang/lib/CodeGen/MicrosoftCXXABI.cpp:3121-3123
+  This = This.withElementType(CGM.Int8Ty);
   llvm::Value *VBPtr = Builder.CreateInBoundsGEP(
   This.getElementType(), This.getPointer(), VBPtrOffset, "vbptr");





Comment at: clang/lib/CodeGen/MicrosoftCXXABI.cpp:3126
   VBPtr = Builder.CreateBitCast(VBPtr,
 
CGM.Int32Ty->getPointerTo(0)->getPointerTo(This.getAddressSpace()));
 

Can drop this bitcast while here.



Comment at: clang/lib/CodeGen/Targets/AArch64.cpp:523
 Address Addr = Address(Load, CGF.Int8Ty, SlotSize);
-return CGF.Builder.CreateElementBitCast(Addr, CGF.ConvertTypeForMem(Ty));
+return Addr.withElementType(CGF.ConvertTypeForMem(Ty));
   }

Can inline this withElementType call into Address ctor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153314

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

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: llvm/utils/UpdateTestChecks/common.py:1286
+  if value == default_value:
+continue
 if action.dest == 'filters':

nikic wrote:
> nikic wrote:
> > hnrklssn wrote:
> > > nikic wrote:
> > > > We should also not print the `all` argument for `--check-globals` 
> > > > argument for `version < 3`, otherwise that will introduce a spurious 
> > > > change in all such tests.
> > > I don't know how to dynamically change the `--check-globals` option 
> > > between `store_true` and `choices`, since the behaviour can itself be 
> > > affected by which argument is passed to the `--version` option. So the 
> > > way I see it there's no way of knowing how to parse between two different 
> > > option types ahead of time. For the default when no option is specified 
> > > the parsing is the same however, so we can simply infer later what option 
> > > is implied based on the version.
> > It's not necessary to change the option, just how it is printed. I.e. add 
> > something like this:
> > ```
> > if args.version < 3 and value == "all":
> > # Don't include argument value in older versions.
> > autogenerated_note_args += "--check-globals "
> > continue
> > ```
> Ah, I get what you're saying now -- old UTC_ARGS are not accepted at all 
> anymore, and trying to update existing tests will just fail with an error!
> 
> We do need old UTC_ARGS to work. I think you can make the argument value 
> optional using these options:
> ```
> nargs="?", 
> const="default",
> default="default",
> choices=["none", "smart", "all", "default"],
> ```
> 
> Or possibly omit const/default and handle None instead.
Correction, I think the right options are:
```
nargs="?",
const="all",
default="default",
choices=["none", "smart", "all"],
```
That is, use `"default"` is it's not specified at all, use `"all"` if only 
`--check-globals` is passed and also accept `--check-globals none|smart|all`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148216

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

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: llvm/utils/UpdateTestChecks/common.py:1286
+  if value == default_value:
+continue
 if action.dest == 'filters':

nikic wrote:
> hnrklssn wrote:
> > nikic wrote:
> > > We should also not print the `all` argument for `--check-globals` 
> > > argument for `version < 3`, otherwise that will introduce a spurious 
> > > change in all such tests.
> > I don't know how to dynamically change the `--check-globals` option between 
> > `store_true` and `choices`, since the behaviour can itself be affected by 
> > which argument is passed to the `--version` option. So the way I see it 
> > there's no way of knowing how to parse between two different option types 
> > ahead of time. For the default when no option is specified the parsing is 
> > the same however, so we can simply infer later what option is implied based 
> > on the version.
> It's not necessary to change the option, just how it is printed. I.e. add 
> something like this:
> ```
> if args.version < 3 and value == "all":
> # Don't include argument value in older versions.
> autogenerated_note_args += "--check-globals "
> continue
> ```
Ah, I get what you're saying now -- old UTC_ARGS are not accepted at all 
anymore, and trying to update existing tests will just fail with an error!

We do need old UTC_ARGS to work. I think you can make the argument value 
optional using these options:
```
nargs="?", 
const="default",
default="default",
choices=["none", "smart", "all", "default"],
```

Or possibly omit const/default and handle None instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148216

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

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: llvm/utils/UpdateTestChecks/common.py:1286
+  if value == default_value:
+continue
 if action.dest == 'filters':

hnrklssn wrote:
> nikic wrote:
> > We should also not print the `all` argument for `--check-globals` argument 
> > for `version < 3`, otherwise that will introduce a spurious change in all 
> > such tests.
> I don't know how to dynamically change the `--check-globals` option between 
> `store_true` and `choices`, since the behaviour can itself be affected by 
> which argument is passed to the `--version` option. So the way I see it 
> there's no way of knowing how to parse between two different option types 
> ahead of time. For the default when no option is specified the parsing is the 
> same however, so we can simply infer later what option is implied based on 
> the version.
It's not necessary to change the option, just how it is printed. I.e. add 
something like this:
```
if args.version < 3 and value == "all":
# Don't include argument value in older versions.
autogenerated_note_args += "--check-globals "
continue
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148216

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

[PATCH] D150997: [llvm] Split out DenseMapInfo specialization

[Nikita Popov via Phabricator via cfe-commits]

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

LGTM. The diff looks weird for some reason, but the downloadable `.diff` file 
looks fine.


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

https://reviews.llvm.org/D150997

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

[PATCH] D153196: [clang] Replace uses of CGBuilderTy::CreateElementBitCast (NFC)

[Nikita Popov via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/lib/CodeGen/CGObjC.cpp:2207
   llvm::Type *origType = addr.getElementType();
-  addr = CGF.Builder.CreateElementBitCast(addr, CGF.Int8PtrTy);
+  addr = addr.withElementType(CGF.Int8PtrTy);
 

Can drop this call, we only use addr.getPointer() below.



Comment at: clang/lib/CodeGen/CGObjC.cpp:2214
   if (origType != CGF.Int8PtrTy)
 result = CGF.Builder.CreateBitCast(result, origType);
 

While here, might as well drop this as well.



Comment at: clang/lib/CodeGen/CGObjC.cpp:2662
   // Cast the argument to 'id*'.
-  addr = Builder.CreateElementBitCast(addr, Int8PtrTy);
+  addr = addr.withElementType(Int8PtrTy);
 

Can drop this, as we only use `addr.getPointer()` below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153196

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

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

Could you please rebase the patch to current main? I wasn't able to apply it.

The behavior you describe sounds reasonable to me.




Comment at: llvm/utils/UpdateTestChecks/common.py:1286
+  if value == default_value:
+continue
 if action.dest == 'filters':

We should also not print the `all` argument for `--check-globals` argument for 
`version < 3`, otherwise that will introduce a spurious change in all such 
tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148216

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

[PATCH] D152999: [CGTypes] Remove recursion protection

[Nikita Popov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2903a8ab0726: [CGTypes] Remove recursion protection 
(authored by nikic).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152999

Files:
  clang/lib/CodeGen/CodeGenTypes.cpp
  clang/lib/CodeGen/CodeGenTypes.h

Index: clang/lib/CodeGen/CodeGenTypes.h
===
--- clang/lib/CodeGen/CodeGenTypes.h
+++ clang/lib/CodeGen/CodeGenTypes.h
@@ -78,20 +78,12 @@
   /// Hold memoized CGFunctionInfo results.
   llvm::FoldingSet FunctionInfos{FunctionInfosLog2InitSize};
 
-  /// This set keeps track of records that we're currently converting
-  /// to an IR type.  For example, when converting:
-  /// struct A { struct B { int x; } } when processing 'x', the 'A' and 'B'
-  /// types will be in this set.
-  llvm::SmallPtrSet RecordsBeingLaidOut;
-
   llvm::SmallPtrSet FunctionsBeingProcessed;
 
   /// True if we didn't layout a function due to a being inside
   /// a recursive struct conversion, set this to true.
   bool SkippedLayout;
 
-  SmallVector DeferredRecords;
-
   /// This map keeps cache of llvm::Types and maps clang::Type to
   /// corresponding llvm::Type.
   llvm::DenseMap TypeCache;
@@ -300,12 +292,6 @@
   bool isZeroInitializable(const RecordDecl *RD);
 
   bool isRecordLayoutComplete(const Type *Ty) const;
-  bool noRecordsBeingLaidOut() const {
-return RecordsBeingLaidOut.empty();
-  }
-  bool isRecordBeingLaidOut(const Type *Ty) const {
-return RecordsBeingLaidOut.count(Ty);
-  }
   unsigned getTargetAddressSpace(QualType T) const;
 };
 
Index: clang/lib/CodeGen/CodeGenTypes.cpp
===
--- clang/lib/CodeGen/CodeGenTypes.cpp
+++ clang/lib/CodeGen/CodeGenTypes.cpp
@@ -125,93 +125,9 @@
   return I != RecordDeclTypes.end() && !I->second->isOpaque();
 }
 
-static bool
-isSafeToConvert(QualType T, CodeGenTypes &CGT,
-llvm::SmallPtrSet &AlreadyChecked);
-
-
-/// isSafeToConvert - Return true if it is safe to convert the specified record
-/// decl to IR and lay it out, false if doing so would cause us to get into a
-/// recursive compilation mess.
-static bool
-isSafeToConvert(const RecordDecl *RD, CodeGenTypes &CGT,
-llvm::SmallPtrSet &AlreadyChecked) {
-  // If we have already checked this type (maybe the same type is used by-value
-  // multiple times in multiple structure fields, don't check again.
-  if (!AlreadyChecked.insert(RD).second)
-return true;
-
-  const Type *Key = CGT.getContext().getTagDeclType(RD).getTypePtr();
-
-  // If this type is already laid out, converting it is a noop.
-  if (CGT.isRecordLayoutComplete(Key)) return true;
-
-  // If this type is currently being laid out, we can't recursively compile it.
-  if (CGT.isRecordBeingLaidOut(Key))
-return false;
-
-  // If this type would require laying out bases that are currently being laid
-  // out, don't do it.  This includes virtual base classes which get laid out
-  // when a class is translated, even though they aren't embedded by-value into
-  // the class.
-  if (const CXXRecordDecl *CRD = dyn_cast(RD)) {
-for (const auto &I : CRD->bases())
-  if (!isSafeToConvert(I.getType()->castAs()->getDecl(), CGT,
-   AlreadyChecked))
-return false;
-  }
-
-  // If this type would require laying out members that are currently being laid
-  // out, don't do it.
-  for (const auto *I : RD->fields())
-if (!isSafeToConvert(I->getType(), CGT, AlreadyChecked))
-  return false;
-
-  // If there are no problems, lets do it.
-  return true;
-}
-
-/// isSafeToConvert - Return true if it is safe to convert this field type,
-/// which requires the structure elements contained by-value to all be
-/// recursively safe to convert.
-static bool
-isSafeToConvert(QualType T, CodeGenTypes &CGT,
-llvm::SmallPtrSet &AlreadyChecked) {
-  // Strip off atomic type sugar.
-  if (const auto *AT = T->getAs())
-T = AT->getValueType();
-
-  // If this is a record, check it.
-  if (const auto *RT = T->getAs())
-return isSafeToConvert(RT->getDecl(), CGT, AlreadyChecked);
-
-  // If this is an array, check the elements, which are embedded inline.
-  if (const auto *AT = CGT.getContext().getAsArrayType(T))
-return isSafeToConvert(AT->getElementType(), CGT, AlreadyChecked);
-
-  // Otherwise, there is no concern about transforming this.  We only care about
-  // things that are contained by-value in a structure that can have another
-  // structure as a member.
-  return true;
-}
-
-
-/// isSafeToConvert - Return true if it is safe to convert the specified record
-/// decl to IR and lay it out, false if doing so would cause us to get into a
-/// recursive compilation mess.
-static bool isSafeToCo

[PATCH] D152321: [clang] Replace use of Type::getPointerTo() (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic accepted this revision.
nikic added a comment.

LGTM -- do you have commit access, or should someone commit this for you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152321

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

[PATCH] D152321: [clang] Replace use of Type::getPointerTo() (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

Looks reasonable. Main note I have is that I think the use of getUnqual() over 
passing AddrSpace=0 would be preferred.




Comment at: clang/lib/CodeGen/CGAtomic.cpp:91
 auto Addr = CGF.Builder.CreatePointerBitCastOrAddrSpaceCast(
-VoidPtrAddr, IntTy->getPointerTo(), "atomic_bitfield_base");
+VoidPtrAddr, llvm::PointerType::get(CGF.getLLVMContext(), 0),
+"atomic_bitfield_base");

getUnqual



Comment at: clang/lib/CodeGen/TargetInfo.cpp:2045
   if (IsIndirect)
-LLTy = LLTy->getPointerTo(0);
+LLTy = llvm::PointerType::get(getVMContext(), 0);
   FrameFields.push_back(LLTy);

getLLVMContext?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152321

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

[PATCH] D152505: [Clang] Directly create opaque pointers

[Nikita Popov 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 rG09d6ee765780: [Clang] Directly create opaque pointers 
(authored by nikic).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152505

Files:
  clang/lib/CodeGen/CodeGenTypes.cpp
  clang/test/CodeGenCXX/matrix-type-builtins.cpp
  clang/test/CodeGenCXX/warn-all-padded-packed-packed-non-pod.cpp
  clang/test/Modules/compare-record.c
  clang/test/PCH/headersearch.cpp
  clang/test/Sema/ms_class_layout.cpp

Index: clang/test/Sema/ms_class_layout.cpp
===
--- clang/test/Sema/ms_class_layout.cpp
+++ clang/test/Sema/ms_class_layout.cpp
@@ -147,16 +147,16 @@
   // This avoid "Can't yet mangle constructors!" for MS ABI.
   C* c;
   c->foo();
-  DerivedStruct* v;
-  H* g;
+  DerivedStruct v;
+  H g;
   BaseStruct* u;
-  I* i;
-  N* n;
-  O* o;
-  P* p;
-  R* r;
-  sd *h;
-  EV *j;
+  I i;
+  N n;
+  O o;
+  P p;
+  R r;
+  sd h;
+  EV j;
   return 0;
 }
 
@@ -206,15 +206,6 @@
 // CHECK-NEXT: sizeof=80, align=8
 // CHECK-NEXT: nvsize=64, nvalign=8
 
-// CHECK: %class.D = type { ptr, double }
-
-// CHECK: %class.B = type { ptr, i32 }
-
-// CHECK: %class.A = type { %class.B, i32, i8 }
-
-// CHECK: %class.C = type { %class.D, %class.B, ptr, double, i32, double, i32, [4 x i8], %class.A }
-// CHECK: %class.C.base = type { %class.D, %class.B, ptr, double, i32, double, i32 }
-
 // CHECK-LABEL: 0 | struct BaseStruct{{$}}
 // CHECK-NEXT:  0 |   double v0
 // CHECK-NEXT:  8 |   float v1
@@ -239,8 +230,6 @@
 // CHECK-NEXT: sizeof=96, align=8
 // CHECK-NEXT: nvsize=96, nvalign=8
 
-// CHECK: %struct.BaseStruct = type { double, float, %class.C }
-
 // CHECK-LABEL: 0 | struct DerivedStruct{{$}}
 // CHECK-NEXT:  0 |   struct BaseStruct (base)
 // CHECK-NEXT:  0 | double v0
@@ -267,8 +256,6 @@
 // CHECK-NEXT: sizeof=104, align=8
 // CHECK-NEXT: nvsize=104, nvalign=8
 
-// CHECK: %struct.DerivedStruct = type { %struct.BaseStruct, i32 }
-
 // CHECK-LABEL:0 | struct G
 // CHECK-NEXT: 0 |   int g_field
 // CHECK-NEXT: sizeof=4, align=4
@@ -284,8 +271,6 @@
 // CHECK-NEXT: sizeof=24, align=8
 // CHECK-NEXT: nvsize=8, nvalign=8
 
-// CHECK: %struct.H = type { %struct.G, ptr, %class.D }
-
 // CHECK-LABEL: 0 | struct I{{$}}
 // CHECK-NEXT:  0 |   (I vftable pointer)
 // CHECK-NEXT:  8 |   (I vbtable pointer)
@@ -296,9 +281,6 @@
 // CHECK-NEXT: sizeof=40, align=8
 // CHECK-NEXT: nvsize=24, nvalign=8
 
-// CHECK: %struct.I = type { ptr, [4 x i8], ptr, double, %class.D }
-// CHECK: %struct.I.base = type { ptr, [4 x i8], ptr, double }
-
 // CHECK-LABEL: 0 | struct L{{$}}
 // CHECK-NEXT:  0 |   int l
 // CHECK-NEXT: sizeof=4, align=4
@@ -316,9 +298,6 @@
 // CHECK-NEXT:  8 | int k
 // CHECK-NEXT: sizeof=12, align=4
 
-//CHECK: %struct.M = type { ptr, i32, %struct.K }
-//CHECK: %struct.M.base = type { ptr, i32 }
-
 // CHECK-LABEL: 0 | struct N{{$}}
 // CHECK-NEXT:  0 |   (N vftable pointer)
 // CHECK-NEXT:  4 |   struct L (base)
@@ -331,8 +310,6 @@
 // CHECK-NEXT: sizeof=20, align=4
 // CHECK-NEXT: nvsize=16, nvalign=4
 
-//CHECK: %struct.N = type { ptr, %struct.L, %struct.M.base, %struct.K }
-
 // CHECK-LABEL: 0 | struct O{{$}}
 // CHECK-NEXT:  0 |   (O vftable pointer)
 // CHECK-NEXT:  8 |   struct H (base)
@@ -347,9 +324,6 @@
 // CHECK-NEXT:| [sizeof=40, align=8
 // CHECK-NEXT:|  nvsize=24, nvalign=8]
 
-// CHECK: struct.O = type { ptr, [4 x i8], %struct.H.base, %struct.G, %class.D }
-// CHECK: struct.O.base = type { ptr, [4 x i8], %struct.H.base, %struct.G, [4 x i8] }
-
 // CHECK-LABEL: 0 | struct P{{$}}
 // CHECK-NEXT:  0 |   struct M (base)
 // CHECK-NEXT:  0 | (M vbtable pointer)
@@ -362,14 +336,10 @@
 // CHECK-NEXT: sizeof=20, align=4
 // CHECK-NEXT: nvsize=12, nvalign=4
 
-//CHECK: %struct.P = type { %struct.M.base, i32, %struct.K, %struct.L }
-
 // CHECK-LABEL: 0 | struct R (empty){{$}}
 // CHECK-NEXT:  sizeof=1, align=1
 // CHECK-NEXT:  nvsize=0, nvalign=1
 
-//CHECK: %struct.R = type { i8 }
-
 // CHECK-LABEL: 0 | struct f{{$}}
 // CHECK-NEXT:  0 |   (f vftable pointer)
 // CHECK-NEXT: sizeof=4, align=4
@@ -419,12 +389,6 @@
 // CHECK-NEXT: sizeof=48, align=4
 // CHECK-NEXT: nvsize=12, nvalign=4
 
-// CHECK: %struct.f = type { ptr }
-// CHECK: %struct.s = type { ptr, ptr, i32, i32, %struct.f }
-// CHECK: %class.IA = type { ptr }
-// CHECK: %class.ICh = type { ptr, ptr, i32, %class.IA }
-// CHECK: %struct.sd = type { ptr, i32, i8, i32, %struct.f, %struct.s.base, i32, %class.IA, %class.ICh.base }
-
 // CHECK-LABEL: 0 | struct AV{{$}}
 // CHECK-NEXT:  0 |   (AV vftable pointer)
 // CHECK-NEXT: sizeof=4, align=4
@@ -445,11 +409,6 @@
 // CHECK-NEXT: sizeof=12, align=4
 // CHECK-NEXT: nvsize=4, nvalign=4
 
-// CHECK: %struct.AV = type { ptr }
-// CHECK: %struct.BV = ty

[PATCH] D142823: Intrinsics: Allow tablegen to mark parameters with dereferenceable

[Nikita Popov via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D142823

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

[PATCH] D152752: [MS] Fix passing aligned records by value in some cases

[Nikita Popov via Phabricator via cfe-commits]

nikic accepted this revision.
nikic 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/D152752/new/

https://reviews.llvm.org/D152752

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

[PATCH] D152226: [FunctionAttrs] Propagate some func/arg/ret attributes from caller to callsite (WIP)

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: llvm/lib/Transforms/Utils/InferCallsiteAttrs.cpp:597
+  // callsite violating the constraint.
+  if (checkCallerDoesNotAccessMemory() && !CB->doesNotAccessMemory()) {
+// Wait until we know we actually need it to do potentially expensive

For these you generally want to query `getMemoryEffects()` on the caller/callee 
once and then work on that representation, instead of doing separate queries 
for everything. This may allow more precise handling and likely reduces the 
compile-time impact, as doing these attribute lookups is quite expensive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152226

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

[PATCH] D152522: [NFC][SetVector] Update some usages of SetVector to SmallSetVector

[Nikita Popov via Phabricator via cfe-commits]

nikic accepted this revision.
nikic 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/D152522/new/

https://reviews.llvm.org/D152522

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

[PATCH] D152321: [clang] Replace use of Type::getPointerTo() (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: clang/lib/CodeGen/CGBuilder.h:172
+auto *PtrTy = llvm::PointerType::get(Ty, Addr.getAddressSpace());
 return Address(CreateBitCast(Addr.getPointer(), PtrTy, Name), Ty,
Addr.getAlignment(), Addr.isKnownNonNull());

JOE1994 wrote:
> nikic wrote:
> > Can remove this bit cast.
> Wouldn't removing the bitcast cause behavior change for uses of 
> `CreateElementBitCast` that supply a `Name` that is not `""`?
This will never actually create an instruction, so the name is already ignored 
now. It would make sense to remove the parameter altogether.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152321

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

[PATCH] D152321: [clang] Replace use of Type::getPointerTo() (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D152321#4403719 , @JOE1994 wrote:

> Some tests that run with `-no-opaque-pointers` began failing after I updated 
> the revision to get rid of bitcasts.
> The bitcasts are still needed if running Clang with `-no-opaque-pointers`.

I have finished migrating these tests and removed the flag in 
https://reviews.llvm.org/D152447, so this should no longer be an issue.




Comment at: clang/lib/CodeGen/CGBuilder.h:172
+auto *PtrTy = llvm::PointerType::get(Ty, Addr.getAddressSpace());
 return Address(CreateBitCast(Addr.getPointer(), PtrTy, Name), Ty,
Addr.getAlignment(), Addr.isKnownNonNull());

Can remove this bit cast.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:222
CGF.getContext().getTypeSize(T));
-  llvm::Type *IntPtrType = IntType->getPointerTo(AddrSpace);
+  llvm::Type *IntPtrType = llvm::PointerType::get(IntType, AddrSpace);
 

We should be using the overload that take as a context rather than a type, to 
create an opaque pointers. The variable name should also drop the mention of 
the specific type.

Same for other uses.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:328
   Value *Args[3];
   Args[0] = CGF.Builder.CreateBitCast(DestPtr, IntPtrType);
   Args[1] = CGF.EmitScalarExpr(E->getArg(1));

Remove bitcast



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:421
+  llvm::Type *Int128PtrTy = llvm::PointerType::getUnqual(Int128Ty);
   Destination = CGF.Builder.CreateBitCast(Destination, Int128PtrTy);
   Address ComparandResult(CGF.Builder.CreateBitCast(ComparandPtr, Int128PtrTy),

Remove bitcast



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:4668
 llvm::Value *Destination =
   Builder.CreateBitCast(EmitScalarExpr(E->getArg(0)), IntPtrType);
 

Remove bitcast (more below...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152321

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

[PATCH] D152447: [Clang] Remove -no-opaque-pointers cc1 flag

[Nikita Popov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG066fb7a58c5a: [Clang] Remove -no-opaque-pointers cc1 flag 
(authored by nikic).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D152447?vs=529597&id=529620#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152447

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/test/CodeGen/opaque-pointers-flag.c
  clang/test/CodeGenOpenCL/opaque-ptr-spirv.cl
  llvm/docs/OpaquePointers.rst

Index: llvm/docs/OpaquePointers.rst
===
--- llvm/docs/OpaquePointers.rst
+++ llvm/docs/OpaquePointers.rst
@@ -289,6 +289,8 @@
 The following typed pointer functionality has already been removed:
 
 * The ``CLANG_ENABLE_OPAQUE_POINTERS`` cmake flag is no longer supported.
+* The ``-no-opaque-pointers`` cc1 clang flag is no longer supported.
+* The ``-plugin-opt=no-opaque-pointers`` LTO flag is no longer supported.
 * C APIs that do not support opaque pointers (like ``LLVMBuildLoad``) are no
   longer supported.
 * Typed pointer IR and bitcode is implicitly upgraded to use opaque pointers,
@@ -296,6 +298,5 @@
 
 The following typed pointer functionality is still to be removed:
 
-* The ``-no-opaque-pointers`` cc1 flag, ``-opaque-pointers=0`` opt flag and
-  ``-plugin-opt=no-opaque-pointers`` lto flag.
+* The ``-opaque-pointers=0`` opt flag.
 * Support for typed pointers in LLVM libraries.
Index: clang/test/CodeGenOpenCL/opaque-ptr-spirv.cl
===
--- clang/test/CodeGenOpenCL/opaque-ptr-spirv.cl
+++ /dev/null
@@ -1,10 +0,0 @@
-// RUN: %clang_cc1 -no-opaque-pointers -emit-llvm -o - -triple spirv64 %s | FileCheck %s
-
-// Check that we have a way to recover pointer
-// types for extern function prototypes (see PR56660).
-extern void foo(global int * ptr);
-kernel void k(global int * ptr) {
-  foo(ptr);
-}
-//CHECK: define spir_kernel void @k(i32 {{.*}}*
-//CHECK: declare spir_func void @foo(i32 {{.*}}*
Index: clang/test/CodeGen/opaque-pointers-flag.c
===
--- clang/test/CodeGen/opaque-pointers-flag.c
+++ /dev/null
@@ -1,25 +0,0 @@
-// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s -check-prefix=TYPED
-// RUN: %clang_cc1 -opaque-pointers -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s -check-prefix=OPAQUE
-
-// TYPED-LABEL: @test(
-// TYPED-NEXT:  entry:
-// TYPED-NEXT:[[P_ADDR:%.*]] = alloca ptr, align 8
-// TYPED-NEXT:store ptr [[P:%.*]], ptr [[P_ADDR]], align 8
-// TYPED-NEXT:[[TMP0:%.*]] = load ptr, ptr [[P_ADDR]], align 8
-// TYPED-NEXT:[[ARRAYIDX:%.*]] = getelementptr inbounds i32, ptr [[TMP0]], i64 1
-// TYPED-NEXT:[[TMP1:%.*]] = load i32, ptr [[ARRAYIDX]], align 4
-// TYPED-NEXT:ret i32 [[TMP1]]
-//
-// OPAQUE-LABEL: @test(
-// OPAQUE-NEXT:  entry:
-// OPAQUE-NEXT:[[P_ADDR:%.*]] = alloca ptr, align 8
-// OPAQUE-NEXT:store ptr [[P:%.*]], ptr [[P_ADDR]], align 8
-// OPAQUE-NEXT:[[TMP0:%.*]] = load ptr, ptr [[P_ADDR]], align 8
-// OPAQUE-NEXT:[[ARRAYIDX:%.*]] = getelementptr inbounds i32, ptr [[TMP0]], i64 1
-// OPAQUE-NEXT:[[TMP1:%.*]] = load i32, ptr [[ARRAYIDX]], align 4
-// OPAQUE-NEXT:ret i32 [[TMP1]]
-//
-int test(int *p) {
-  return p[1];
-}
Index: clang/lib/CodeGen/CodeGenAction.cpp
===
--- clang/lib/CodeGen/CodeGenAction.cpp
+++ clang/lib/CodeGen/CodeGenAction.cpp
@@ -1047,8 +1047,6 @@
   if (BA != Backend_EmitNothing && !OS)
 return nullptr;
 
-  VMContext->setOpaquePointers(CI.getCodeGenOpts().OpaquePointers);
-
   // Load bitcode modules to link with, if we need to.
   if (LinkModules.empty())
 for (const CodeGenOptions::BitcodeFileToLink &F :
@@ -1106,8 +1104,6 @@
   CompilerInstance &CI = getCompilerInstance();
   SourceManager &SM = CI.getSourceManager();
 
-  VMContext->setOpaquePointers(CI.getCodeGenOpts().OpaquePointers);
-
   // For ThinLTO backend invocations, ensure that the context
   // merges types based on ODR identifiers. We also need to read
   // the correct module out of a multi-module bitcode file.
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -6029,13 +6029,6 @@
   PosFlag,
   NegFlag,
   BothFlags<[], " analyzing function argument and return types for mandatory definedness">>;
-defm opaque_pointers : BoolOption<"",
-  "opaque-pointers",
-  CodeGenOpts<"Op

[PATCH] D152321: [clang] Replace use of Type::getPointerTo() (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: clang/lib/CodeGen/Address.h:135
 llvm::Constant *BitCast = llvm::ConstantExpr::getBitCast(
-getPointer(), ElemTy->getPointerTo(getAddressSpace()));
+getPointer(), llvm::PointerType::get(ElemTy, getAddressSpace()));
 return ConstantAddress(BitCast, ElemTy, getAlignment());

In these cases, what we want to do is remove the `BitCast`, which is no longer 
necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152321

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

[PATCH] D152023: [UBSan] Consider zero input to __builtin_clz/ctz to be undefined independent of the target.

[Nikita Popov via Phabricator via cfe-commits]

nikic accepted this revision.
nikic 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/D152023/new/

https://reviews.llvm.org/D152023

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

[PATCH] D150670: [InstCombine] Disable generation of fshl/fshr for rotates

[Nikita Popov via Phabricator via cfe-commits]

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

LGTM, let's give it a try. The patch description needs an update though.




Comment at: llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:920
+return I;
+} else { // fshl is a rotate
+  KnownBits LHSKnown = computeKnownBits(I->getOperand(0), Depth + 1, 
I);

Add some more explanation here, something like:
`// Avoid converting rotate into funnel shift. Only simplify if one operand is 
constant.`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150670

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

[PATCH] D150670: [InstCombine] Disable generation of fshl/fshr for rotates

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

Can you please drop all wasm related tests and instead add an InstCombine test 
for the fsh+and pattern?

It would also be good to have a test where we can fold one side to a constant, 
but that constant is not zero. We should then consider whether that is 
profitable or not. (In that case we can't reduce to a simple shift and will 
reduce to a shift and or with constant instead -- is that better or worse than 
a rotate?)




Comment at: llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:933
+Depth + 1)))
+return I;
+}

Calling SimplifyDemandedBits() for the case of known demanded bits is a bit 
odd, I'd probably write something like this instead:
```
KnownBits LHSKnown = computeKnownBits(I->getOperand(0), Depth + 1, I);
if (DemandedMaskLHS.isSubsetOf(LHSKnown.Zero | LHSKnown.One)) {
  replaceOperand(I, 0, Constant::getIntegerValue(VTy, LHSKnown.One);
  return &I;
}
KnownBits RHSKnown = computeKnownBits(I->getOperand(1), Depth + 1, I);
if (DemandedMaskRHS.isSubsetOf(LHSKnown.Zero | RHSKnown.One)) {
  replaceOperand(I, 1, Constant::getIntegerValue(VTy, RHSKnown.One);
  return &I;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150670

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

[PATCH] D151701: [HIP] Add missing __hip_atomic_fetch_sub support

[Nikita Popov via Phabricator via cfe-commits]

nikic resigned from this revision.
nikic added a comment.

(Looks reasonable, but is pretty far outside my area of expertise...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151701

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

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: 
llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:927-933
+if (I->getOperand(0) != I->getOperand(1)) {
+  APInt DemandedMaskLHS(DemandedMask.lshr(ShiftAmt));
+  APInt DemandedMaskRHS(DemandedMask.shl(BitWidth - ShiftAmt));
+  if (SimplifyDemandedBits(I, 0, DemandedMaskLHS, LHSKnown, Depth + 1) 
||
+  SimplifyDemandedBits(I, 1, DemandedMaskRHS, RHSKnown, Depth + 1))
+return I;
+}

You should be able to do something along these lines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150670

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

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: llvm/test/Transforms/InstCombine/fsh.ll:664
+; CHECK-NEXT:[[T1:%.*]] = and i32 [[A:%.*]], -65536
+; CHECK-NEXT:[[T2:%.*]] = call i32 @llvm.fshl.i32(i32 [[T1]], i32 [[T1]], 
i32 16)
 ; CHECK-NEXT:ret i32 [[T2]]

We still want to simplify this case. Could possibly be done by checking whether 
all demanded bits are zero for one of the operands in the rotate case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150670

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

[PATCH] D150887: [clang] Convert a few tests to opaque pointers

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

I wanted to check whether you plan to do more opaque pointer test conversions 
in clang. Just to make sure we don't duplicate work...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150887

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

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: 
clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected:12
+// CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:ret i32 [[TMP1]]
+//

hnrklssn wrote:
> nikic wrote:
> > hnrklssn wrote:
> > > nikic wrote:
> > > > nikic wrote:
> > > > > hnrklssn wrote:
> > > > > > hnrklssn wrote:
> > > > > > > nikic wrote:
> > > > > > > > hnrklssn wrote:
> > > > > > > > > delcypher wrote:
> > > > > > > > > > @hnrklssn I just noticed we don't have a `CHECK` for what 
> > > > > > > > > > `META2` actually refers to. Should we?
> > > > > > > > > > 
> > > > > > > > > > Not something that has to be fixed in this patch, more just 
> > > > > > > > > > an observation.
> > > > > > > > > Indeed this is true for metadata in general, presumably 
> > > > > > > > > because the RHS often refer to things like other metadata 
> > > > > > > > > identifiers. In the case of annotations they seem to always 
> > > > > > > > > refer to simple strings however, so it would be feasible to 
> > > > > > > > > do a straight match without having to do recursive matching 
> > > > > > > > > or complex regexes to determine which part of the metadata to 
> > > > > > > > > match against.
> > > > > > > > > 
> > > > > > > > > In many cases with metadata attached to IR nodes, multiple 
> > > > > > > > > nodes refer to the same metadata node, so at least you verify 
> > > > > > > > > that they still are consistent. But I agree that verifying 
> > > > > > > > > the content would be a great future addition.
> > > > > > > > You need to pass `--check-globals` to check the actual metadata.
> > > > > > > When I add that to this test case it adds
> > > > > > > 
> > > > > > > ```
> > > > > > > //.
> > > > > > > // CHECK: attributes #0 = { noinline nounwind optnone 
> > > > > > > "min-legal-vector-width"="0" "no-trapping-math"="true" 
> > > > > > > "stack-protector-buffer-size"="8" 
> > > > > > > "target-features"="+cx8,+mmx,+sse,+sse2,+x87" }
> > > > > > > //.
> > > > > > > // CHECK: !0 = !{i32 1, !"wchar_size", i32 4}
> > > > > > > // CHECK: !1 = !{!"clang version 17.0.0 
> > > > > > > (g...@github.com:llvm/llvm-project.git 
> > > > > > > 684914f47cf59e9ab6d8b0f73c58ca6272ea28d4)"}
> > > > > > > // CHECK: !2 = !{!"auto-init"}
> > > > > > > //.
> > > > > > > ```
> > > > > > > 
> > > > > > > So it seems to just be doing a simple literal matching on all 
> > > > > > > metadata, regardless of whether we captured that metadata in any 
> > > > > > > filecheck variable. And it still doesn't use the META2 variable 
> > > > > > > to match the definition. Am I missing something? If we use the 
> > > > > > > literal metadata names instead of variable matching for the 
> > > > > > > definitions, there isn't much point in doing variable matching 
> > > > > > > for the metadata uses either, since the test still very much 
> > > > > > > relies on the metadata numbering being stable.
> > > > > > @nikic Do you have more information to add about how metadata 
> > > > > > definition matchers can be generated without hardcoding everything 
> > > > > > (which is kind of the opposite of what this patch is trying to do), 
> > > > > > or in general if you're happy with the state of the PR?
> > > > > This works fine with update_test_checks, so it must be some bug in 
> > > > > update_cc_test_checks in particular. From a quick look, I suspect 
> > > > > it's because 
> > > > > https://github.com/llvm/llvm-project/blob/3d05ab6d3e24e76ff53b8d7d623c436b4be5b809/llvm/utils/update_cc_test_checks.py#L447
> > > > >  hardcodes a True value, while update_test_checks makes this 
> > > > > dependent on `--preserve-names`, which is disabled by default there.
> > > > Or maybe just test this with update_test_checks? This change is 
> > > > valuable for that script as well, and it doesn't have this extra issue.
> > > I couldn't find any uses of 
> > > `--preserve-names` in the entire repo (not even in tests for 
> > > update_test_checks), so I went ahead and changed update_cc_test_checks to 
> > > pass True instead.
> > > 
> > > While at it I added 2 new options to match some, but not necessarily all, 
> > > globals. I set the least verbose one (other than "none") as the default, 
> > > emitting checks for the definitions of all globals that we emit matches 
> > > for in the IR. Do you agree that this is reasonable?
> > That sounds like a nice approach. My main question would be whether 
> > `--check-globals seen` is the right default: My gut feeling here is that 
> > this will introduce additional checks for attributes/metadata in tests that 
> > don't actually care about it, but I'm not particularly sure about that.
> > 
> > You might want to do some mass test regeneration and take a look over the 
> > diff to see how useful the changes would be.
> > 
> > If we do change the default, the default change likely need to be part of 
> > `--version 3` to avoid substantial test churn.
> I've regene

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

Here are two inputs that currently fold down to constants and no longer do so 
with this patch: https://gist.github.com/nikic/4a714ea550bf2252543570585f642af2 
These need further reduction for PhaseOrdering tests, but should be a good 
starting point for analysis...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145265

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

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

Unfortunately I'm seeing a number of Rust optimization regressions with this 
change. I'll try to reduce those to some PhaseOrdering tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145265

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

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: 
clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected:12
+// CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:ret i32 [[TMP1]]
+//

hnrklssn wrote:
> nikic wrote:
> > nikic wrote:
> > > hnrklssn wrote:
> > > > hnrklssn wrote:
> > > > > nikic wrote:
> > > > > > hnrklssn wrote:
> > > > > > > delcypher wrote:
> > > > > > > > @hnrklssn I just noticed we don't have a `CHECK` for what 
> > > > > > > > `META2` actually refers to. Should we?
> > > > > > > > 
> > > > > > > > Not something that has to be fixed in this patch, more just an 
> > > > > > > > observation.
> > > > > > > Indeed this is true for metadata in general, presumably because 
> > > > > > > the RHS often refer to things like other metadata identifiers. In 
> > > > > > > the case of annotations they seem to always refer to simple 
> > > > > > > strings however, so it would be feasible to do a straight match 
> > > > > > > without having to do recursive matching or complex regexes to 
> > > > > > > determine which part of the metadata to match against.
> > > > > > > 
> > > > > > > In many cases with metadata attached to IR nodes, multiple nodes 
> > > > > > > refer to the same metadata node, so at least you verify that they 
> > > > > > > still are consistent. But I agree that verifying the content 
> > > > > > > would be a great future addition.
> > > > > > You need to pass `--check-globals` to check the actual metadata.
> > > > > When I add that to this test case it adds
> > > > > 
> > > > > ```
> > > > > //.
> > > > > // CHECK: attributes #0 = { noinline nounwind optnone 
> > > > > "min-legal-vector-width"="0" "no-trapping-math"="true" 
> > > > > "stack-protector-buffer-size"="8" 
> > > > > "target-features"="+cx8,+mmx,+sse,+sse2,+x87" }
> > > > > //.
> > > > > // CHECK: !0 = !{i32 1, !"wchar_size", i32 4}
> > > > > // CHECK: !1 = !{!"clang version 17.0.0 
> > > > > (g...@github.com:llvm/llvm-project.git 
> > > > > 684914f47cf59e9ab6d8b0f73c58ca6272ea28d4)"}
> > > > > // CHECK: !2 = !{!"auto-init"}
> > > > > //.
> > > > > ```
> > > > > 
> > > > > So it seems to just be doing a simple literal matching on all 
> > > > > metadata, regardless of whether we captured that metadata in any 
> > > > > filecheck variable. And it still doesn't use the META2 variable to 
> > > > > match the definition. Am I missing something? If we use the literal 
> > > > > metadata names instead of variable matching for the definitions, 
> > > > > there isn't much point in doing variable matching for the metadata 
> > > > > uses either, since the test still very much relies on the metadata 
> > > > > numbering being stable.
> > > > @nikic Do you have more information to add about how metadata 
> > > > definition matchers can be generated without hardcoding everything 
> > > > (which is kind of the opposite of what this patch is trying to do), or 
> > > > in general if you're happy with the state of the PR?
> > > This works fine with update_test_checks, so it must be some bug in 
> > > update_cc_test_checks in particular. From a quick look, I suspect it's 
> > > because 
> > > https://github.com/llvm/llvm-project/blob/3d05ab6d3e24e76ff53b8d7d623c436b4be5b809/llvm/utils/update_cc_test_checks.py#L447
> > >  hardcodes a True value, while update_test_checks makes this dependent on 
> > > `--preserve-names`, which is disabled by default there.
> > Or maybe just test this with update_test_checks? This change is valuable 
> > for that script as well, and it doesn't have this extra issue.
> I couldn't find any uses of 
> `--preserve-names` in the entire repo (not even in tests for 
> update_test_checks), so I went ahead and changed update_cc_test_checks to 
> pass True instead.
> 
> While at it I added 2 new options to match some, but not necessarily all, 
> globals. I set the least verbose one (other than "none") as the default, 
> emitting checks for the definitions of all globals that we emit matches for 
> in the IR. Do you agree that this is reasonable?
That sounds like a nice approach. My main question would be whether 
`--check-globals seen` is the right default: My gut feeling here is that this 
will introduce additional checks for attributes/metadata in tests that don't 
actually care about it, but I'm not particularly sure about that.

You might want to do some mass test regeneration and take a look over the diff 
to see how useful the changes would be.

If we do change the default, the default change likely need to be part of 
`--version 3` to avoid substantial test churn.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148216

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

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D150670#4368241 , @pmatos wrote:

> In D150670#4368238 , @pmatos wrote:
>
>> In D150670#4352163 , @nikic wrote:
>>
>>> 1. Say that we prefer preserving rotates over "simplifying" funnel shifts 
>>> (ending up with the rot2 pattern). Basically by skipping the optimization 
>>> at 
>>> https://github.com/llvm/llvm-project/blob/7f54b38e28b3b66195de672848f2b5366d0d51e3/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp#L927-L931
>>>  if both fsh operands are the same. Assuming this doesn't cause test 
>>> regressions, I think this would be acceptable to do. From a backend 
>>> perspective, even for targets that have a native funnel shift (aarch64, 
>>> x86), the difference between the rot1/rot2 patterns looks pretty neutral.
>
> OK, I just re-read your comment above and I am starting to assume that what 
> you mean is skipping the optimization for all targets if the funnel shift is 
> a rotate (i.e. same first two operands). Is this correct?

That's right.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150670

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

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: 
clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected:12
+// CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:ret i32 [[TMP1]]
+//

nikic wrote:
> hnrklssn wrote:
> > hnrklssn wrote:
> > > nikic wrote:
> > > > hnrklssn wrote:
> > > > > delcypher wrote:
> > > > > > @hnrklssn I just noticed we don't have a `CHECK` for what `META2` 
> > > > > > actually refers to. Should we?
> > > > > > 
> > > > > > Not something that has to be fixed in this patch, more just an 
> > > > > > observation.
> > > > > Indeed this is true for metadata in general, presumably because the 
> > > > > RHS often refer to things like other metadata identifiers. In the 
> > > > > case of annotations they seem to always refer to simple strings 
> > > > > however, so it would be feasible to do a straight match without 
> > > > > having to do recursive matching or complex regexes to determine which 
> > > > > part of the metadata to match against.
> > > > > 
> > > > > In many cases with metadata attached to IR nodes, multiple nodes 
> > > > > refer to the same metadata node, so at least you verify that they 
> > > > > still are consistent. But I agree that verifying the content would be 
> > > > > a great future addition.
> > > > You need to pass `--check-globals` to check the actual metadata.
> > > When I add that to this test case it adds
> > > 
> > > ```
> > > //.
> > > // CHECK: attributes #0 = { noinline nounwind optnone 
> > > "min-legal-vector-width"="0" "no-trapping-math"="true" 
> > > "stack-protector-buffer-size"="8" 
> > > "target-features"="+cx8,+mmx,+sse,+sse2,+x87" }
> > > //.
> > > // CHECK: !0 = !{i32 1, !"wchar_size", i32 4}
> > > // CHECK: !1 = !{!"clang version 17.0.0 
> > > (g...@github.com:llvm/llvm-project.git 
> > > 684914f47cf59e9ab6d8b0f73c58ca6272ea28d4)"}
> > > // CHECK: !2 = !{!"auto-init"}
> > > //.
> > > ```
> > > 
> > > So it seems to just be doing a simple literal matching on all metadata, 
> > > regardless of whether we captured that metadata in any filecheck 
> > > variable. And it still doesn't use the META2 variable to match the 
> > > definition. Am I missing something? If we use the literal metadata names 
> > > instead of variable matching for the definitions, there isn't much point 
> > > in doing variable matching for the metadata uses either, since the test 
> > > still very much relies on the metadata numbering being stable.
> > @nikic Do you have more information to add about how metadata definition 
> > matchers can be generated without hardcoding everything (which is kind of 
> > the opposite of what this patch is trying to do), or in general if you're 
> > happy with the state of the PR?
> This works fine with update_test_checks, so it must be some bug in 
> update_cc_test_checks in particular. From a quick look, I suspect it's 
> because 
> https://github.com/llvm/llvm-project/blob/3d05ab6d3e24e76ff53b8d7d623c436b4be5b809/llvm/utils/update_cc_test_checks.py#L447
>  hardcodes a True value, while update_test_checks makes this dependent on 
> `--preserve-names`, which is disabled by default there.
Or maybe just test this with update_test_checks? This change is valuable for 
that script as well, and it doesn't have this extra issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148216

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

[PATCH] D150997: [llvm] Split out DenseMapInfo specialization

[Nikita Popov via Phabricator via cfe-commits]

nikic requested changes to this revision.
nikic added a comment.
This revision now requires changes to proceed.

(Moving this off the review queue per above comments. There is a missing 
include in a unit test.)


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

https://reviews.llvm.org/D150997

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

[PATCH] D151195: [Driver] Fix test for use of ld from devtoolset

[Nikita Popov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG75cce50fd2d3: [Driver] Fix test for use of ld from 
devtoolset (NFC) (authored by nikic).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151195

Files:
  clang/test/Driver/Inputs/rhel_7_tree/opt/rh/devtoolset-7/root/usr/bin/ld
  
clang/test/Driver/Inputs/rhel_7_tree/opt/rh/devtoolset-7/root/usr/lib/gcc/x86_64-redhat-linux/7/crtbegin.o
  
clang/test/Driver/Inputs/rhel_7_tree/opt/rh/devtoolset-7/root/usr/lib/gcc/x86_64-redhat-linux/7/crtend.o
  clang/test/Driver/linux-ld.c


Index: clang/test/Driver/linux-ld.c
===
--- clang/test/Driver/linux-ld.c
+++ clang/test/Driver/linux-ld.c
@@ -1794,12 +1794,11 @@
 // RUN: %clang -### %s -no-pie 2>&1 \
 // RUN: --target=x86_64-unknown-linux-gnu \
 // RUN: 
--gcc-toolchain="%S/Inputs/rhel_7_tree/opt/rh/devtoolset-7/root/usr" \
-// RUN: --sysroot=%S/Inputs/rhel_7_tree \
+// RUN: --sysroot="%S/Inputs/rhel_7_tree/opt/rh/devtoolset-7/root" \
 // RUN:   | FileCheck --check-prefix=CHECK-LD-RHEL7-DTS %s
-// CHECK-LD-RHEL7-DTS: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
-// CHECK-LD-RHLE7-DTS: Selected GCC installation: 
[[GCC_INSTALL:[[SYSROOT]]/lib/gcc/x86_64-redhat-linux/7]]
+// CHECK-LD-RHEL7-DTS: "-isysroot" "[[SYSROOT:[^"]+]]"
 // CHECK-LD-RHEL7-DTS-NOT: /usr/bin/ld
-// CHECK-LD-RHLE7-DTS: [[GCC_INSTALL]/../../../bin/ld
+// CHECK-LD-RHEL7-DTS: 
[[SYSROOT]]/usr/lib/gcc/x86_64-redhat-linux/7/../../../../bin/ld
 
 // Check whether gcc7 install works fine on Amazon Linux AMI
 // RUN: %clang -### %s -no-pie 2>&1 \


Index: clang/test/Driver/linux-ld.c
===
--- clang/test/Driver/linux-ld.c
+++ clang/test/Driver/linux-ld.c
@@ -1794,12 +1794,11 @@
 // RUN: %clang -### %s -no-pie 2>&1 \
 // RUN: --target=x86_64-unknown-linux-gnu \
 // RUN: --gcc-toolchain="%S/Inputs/rhel_7_tree/opt/rh/devtoolset-7/root/usr" \
-// RUN: --sysroot=%S/Inputs/rhel_7_tree \
+// RUN: --sysroot="%S/Inputs/rhel_7_tree/opt/rh/devtoolset-7/root" \
 // RUN:   | FileCheck --check-prefix=CHECK-LD-RHEL7-DTS %s
-// CHECK-LD-RHEL7-DTS: "{{.*}}ld{{(.exe)?}}" "--sysroot=[[SYSROOT:[^"]+]]"
-// CHECK-LD-RHLE7-DTS: Selected GCC installation: [[GCC_INSTALL:[[SYSROOT]]/lib/gcc/x86_64-redhat-linux/7]]
+// CHECK-LD-RHEL7-DTS: "-isysroot" "[[SYSROOT:[^"]+]]"
 // CHECK-LD-RHEL7-DTS-NOT: /usr/bin/ld
-// CHECK-LD-RHLE7-DTS: [[GCC_INSTALL]/../../../bin/ld
+// CHECK-LD-RHEL7-DTS: [[SYSROOT]]/usr/lib/gcc/x86_64-redhat-linux/7/../../../../bin/ld
 
 // Check whether gcc7 install works fine on Amazon Linux AMI
 // RUN: %clang -### %s -no-pie 2>&1 \
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: 
clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected:12
+// CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:ret i32 [[TMP1]]
+//

hnrklssn wrote:
> hnrklssn wrote:
> > nikic wrote:
> > > hnrklssn wrote:
> > > > delcypher wrote:
> > > > > @hnrklssn I just noticed we don't have a `CHECK` for what `META2` 
> > > > > actually refers to. Should we?
> > > > > 
> > > > > Not something that has to be fixed in this patch, more just an 
> > > > > observation.
> > > > Indeed this is true for metadata in general, presumably because the RHS 
> > > > often refer to things like other metadata identifiers. In the case of 
> > > > annotations they seem to always refer to simple strings however, so it 
> > > > would be feasible to do a straight match without having to do recursive 
> > > > matching or complex regexes to determine which part of the metadata to 
> > > > match against.
> > > > 
> > > > In many cases with metadata attached to IR nodes, multiple nodes refer 
> > > > to the same metadata node, so at least you verify that they still are 
> > > > consistent. But I agree that verifying the content would be a great 
> > > > future addition.
> > > You need to pass `--check-globals` to check the actual metadata.
> > When I add that to this test case it adds
> > 
> > ```
> > //.
> > // CHECK: attributes #0 = { noinline nounwind optnone 
> > "min-legal-vector-width"="0" "no-trapping-math"="true" 
> > "stack-protector-buffer-size"="8" 
> > "target-features"="+cx8,+mmx,+sse,+sse2,+x87" }
> > //.
> > // CHECK: !0 = !{i32 1, !"wchar_size", i32 4}
> > // CHECK: !1 = !{!"clang version 17.0.0 
> > (g...@github.com:llvm/llvm-project.git 
> > 684914f47cf59e9ab6d8b0f73c58ca6272ea28d4)"}
> > // CHECK: !2 = !{!"auto-init"}
> > //.
> > ```
> > 
> > So it seems to just be doing a simple literal matching on all metadata, 
> > regardless of whether we captured that metadata in any filecheck variable. 
> > And it still doesn't use the META2 variable to match the definition. Am I 
> > missing something? If we use the literal metadata names instead of variable 
> > matching for the definitions, there isn't much point in doing variable 
> > matching for the metadata uses either, since the test still very much 
> > relies on the metadata numbering being stable.
> @nikic Do you have more information to add about how metadata definition 
> matchers can be generated without hardcoding everything (which is kind of the 
> opposite of what this patch is trying to do), or in general if you're happy 
> with the state of the PR?
This works fine with update_test_checks, so it must be some bug in 
update_cc_test_checks in particular. From a quick look, I suspect it's because 
https://github.com/llvm/llvm-project/blob/3d05ab6d3e24e76ff53b8d7d623c436b4be5b809/llvm/utils/update_cc_test_checks.py#L447
 hardcodes a True value, while update_test_checks makes this dependent on 
`--preserve-names`, which is disabled by default there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148216

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

[PATCH] D150997: [llvm] Split out DenseMapInfo specialization

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

The pre-merge checks have a build failure in clang-tools-extra.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150997

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

[PATCH] D145403: [Pipeline] Don't run EarlyFPM in LTO post link

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

Any reason why this hasn't been landed yet?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145403

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

[PATCH] D150887: [clang] Convert a few tests to opaque pointers

[Nikita Popov via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/test/CodeGenCXX/const-init-cxx11.cpp:353
   };
-  // CHECK: @_ZN14VirtualMembersL13sGlobalMemoryE = internal global { i8** } { 
i8** getelementptr inbounds ({ [3 x i8*] }, { [3 x i8*] }* 
@_ZTVN14VirtualMembers12nsMemoryImplE, i32 0, inrange i32 0, i32 2) }
+  // CHECK: @_ZN14VirtualMembersL13sGlobalMemoryE = internal global 
%"struct.VirtualMembers::nsMemoryImpl" { ptr getelementptr inbounds ({ [3 x 
ptr] }, ptr @_ZTVN14VirtualMembers12nsMemoryImplE, i32 0, inrange i32 0, i32 2) 
}
   __attribute__((used))

barannikov88 wrote:
> This was one suspicious change. An anonymous struct became named.
> 
Not sure why exactly this happened, but should be harmless. I believe we 
sometimes generate anon structs for initializaton because the types used for 
initialization are not always compatible with the nominal LLVM memory type -- I 
guess there previously was a mismatch in pointer types here or something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150887

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

[PATCH] D150829: [clang] Convert several tests to opaque pointers

[Nikita Popov via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/test/CodeGenCXX/2011-12-19-init-list-ctor.cpp:22
+// CHECK: store i32 0, ptr @arr
+// CHECK: call void @_ZN1AC1EPKc(ptr {{[^,]*}} getelementptr inbounds 
(%struct.S, ptr @arr, i32 0, i32 1), ptr noundef @.str)
+// CHECK: store i32 1, ptr getelementptr inbounds (%struct.S, ptr @arr, i64 1)

barannikov88 wrote:
> This looks suspicious to me. The first gep index has i32 type (used to be 
> i64). The two other geps have it i64.
> 
This is okay -- it's an artifact of using a DL-unaware constant folder in Clang 
CodeGen. Index types will get canonicalized once a DL-aware folder sees them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150829

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

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D150670#4352055 , @pmatos wrote:

> In D150670#4351147 , @nikic wrote:
>
>> This doesn't looks like a wasm specific problem. You get essentially the 
>> same issue on any target that has a rotate instruction but no funnel shift 
>> instruction. Here are just a couple examples: https://godbolt.org/z/8v6nfaax9
>
> Yes, I am indeed aware this is not specific to wasm. What's specific to wasm 
> afaiu is that the code generated is much worse when expanding fshl. That's 
> what I mentioned in the bug discussion here: 
> https://github.com/llvm/llvm-project/issues/62703#issuecomment-1548474310
>
>> I believe this needs to be either solved by preventing demanded bits 
>> simplifications that break a rotate pattern (though I'm not sure if that 
>> would break any other optimizations we care about) or by adding a special 
>> case for this in the backend when lowering FSH to ROT.
>
> Preventing the simplification means adding target specific code in 
> instcombine which seems even worse than adding it here given as @dschuff 
> pointed out, there's precedent with x86.

I'm not suggesting to add any target specific code to instcombine. I think 
there are actually quite a few different ways this could be solved. See 
https://llvm.godbolt.org/z/f55K7K17W for three possible representations of the 
same rotate pattern.

1. Say that we prefer preserving rotates over "simplifying" funnel shifts 
(ending up with the rot2 pattern). Basically by skipping the optimization at 
https://github.com/llvm/llvm-project/blob/7f54b38e28b3b66195de672848f2b5366d0d51e3/llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp#L927-L931
 if both fsh operands are the same. Assuming this doesn't cause test 
regressions, I think this would be acceptable to do. From a backend 
perspective, even for targets that have a native funnel shift (aarch64, x86), 
the difference between the rot1/rot2 patterns looks pretty neutral.

2. Undo the transform in the backend. It is bidrectional 
(https://alive2.llvm.org/ce/z/Chb85F), so this is possible.  This would need an 
extra legalization/combiner pattern (depending on where we form ROTs). 
Advantage of undoing the pattern is the usual one: Works if it was in the 
undesirable form in the first place. (E.g. this could happen if the rotate did 
not use a builtin but was implemented as `x << 8 | x >> 24`, which is probably 
much more widespread than the builtin. Though I checked, and we don't form a 
funnel shift for it in this case right now.)

3. Move the and from the fsh arguments to the result. This is the rot3 pattern. 
This seems to produce the best codegen on average, because it can use uxtb16 on 
ARM. Moving the and from args to return is a bit unusual for unary ops, but if 
we see this as moving two ands on both fsh arguments (which happen to be the 
same) to one on the result, that would be a pretty standard transform.

I think all of those options are viable, and couldn't say for certain which one 
is best. I think any of them would be better than making clang emit a special 
intrinsic just for wasm though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150670

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

[PATCH] D145265: [Pipeline] Remove GlobalCleanupPM

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:476
 
+  FPM.addPass(EarlyCSEPass());
+

Why the extra EarlyCSE pass in the `O1` pipeline?



Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:547
+  SimplifyCFGPass(SimplifyCFGOptions().convertSwitchRangeToICmp(true)));
+  FPM.addPass(AggressiveInstCombinePass());
 

Any particular reason why InstCombine and AggressiveInstCombine are no longer 
directly next to each other?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145265

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

[PATCH] D150670: [WebAssembly] Disable generation of fshl/fshr for rotates

[Nikita Popov via Phabricator via cfe-commits]

nikic requested changes to this revision.
nikic added a comment.
This revision now requires changes to proceed.

This doesn't looks like a wasm specific problem. You get essentially the same 
issue on any target that has a rotate instruction but no funnel shift 
instruction. Here are just a couple examples: https://godbolt.org/z/8v6nfaax9

I believe this needs to be either solved by preventing demanded bits 
simplifications that break a rotate pattern (though I'm not sure if that would 
break any other optimizations we care about) or by adding a special case for 
this in the backend when lowering FSH to ROT.

Lowering to a rotate intrinsic only "solves" this for wasm and will at the same 
time make these rotates completely opaque to optimization -- heck, it looks 
like we don't even support constant folding for these intrinsics 
(https://llvm.godbolt.org/z/hMWG16b9W).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150670

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

[PATCH] D150733: [clang] Convert remaining OpenMP tests to opaque pointers

[Nikita Popov via Phabricator via cfe-commits]

nikic accepted this revision.
nikic 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/D150733/new/

https://reviews.llvm.org/D150733

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

[PATCH] D150694: [clang] Convert NVPTX OpenMP tests to opaque pointers

[Nikita Popov via Phabricator via cfe-commits]

nikic accepted this revision.
nikic 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/D150694/new/

https://reviews.llvm.org/D150694

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

[PATCH] D150704: [clang] Convert several smaller OpenMP tests to opaque pointers

[Nikita Popov via Phabricator via cfe-commits]

nikic accepted this revision.
nikic 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/D150704/new/

https://reviews.llvm.org/D150704

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

[PATCH] D150682: [clang] Convert a few OpenMP tests to opaque pointers

[Nikita Popov via Phabricator via cfe-commits]

nikic accepted this revision.
nikic 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/D150682/new/

https://reviews.llvm.org/D150682

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

[PATCH] D150680: [clang] Convert a few OpenMP tests to opaque pointers

[Nikita Popov via Phabricator via cfe-commits]

nikic accepted this revision.
nikic 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/D150680/new/

https://reviews.llvm.org/D150680

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

[PATCH] D150652: [clang] Convert a few OpenMP tests to opaque pointers

[Nikita Popov via Phabricator via cfe-commits]

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

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150652

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

[PATCH] D150530: [clang] Convert a few OpenMP tests to use opaque pointers

[Nikita Popov via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/test/OpenMP/atomic_capture_codegen.cpp:868
-// CHECK: [[NEW_BF_VALUE:%.+]] = load i8, i8* [[BITCAST1]]
-// CHECK: [[RES:%.+]] = cmpxchg i8* getelementptr inbounds 
(%struct.BitFields4_packed, %struct.BitFields4_packed* @{{.+}}, i32 0, i32 0, 
i64 2), i8 [[OLD_BF_VALUE]], i8 [[NEW_BF_VALUE]] monotonic monotonic, align 1
 // CHECK: [[FAILED_OLD_VAL]] = extractvalue { i8, i1 } [[RES]], 0

barannikov88 wrote:
> Some GEPs like this one have changed the base type and are new missing 
> inbounds keyword, is that OK?
> 
Yes, this is fine. It's a known weakness in constant folding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150530

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

[PATCH] D150520: [clang] Convert a few tests to opaque pointers

[Nikita Popov via Phabricator via cfe-commits]

nikic accepted this revision.
nikic added a comment.

Still LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150520

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

[PATCH] D150520: [clang] Convert a few tests to opaque pointers

[Nikita Popov via Phabricator via cfe-commits]

nikic accepted this revision.
nikic 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/D150520/new/

https://reviews.llvm.org/D150520

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

[PATCH] D145788: [CodeGen] Only consider innermost cast for !heapallocsite

[Nikita Popov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGcac4d7ff4652: [CodeGen] Only consider innermost cast for 
!heapallocsite (authored by nikic).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145788

Files:
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/test/CodeGen/debug-info-codeview-heapallocsite.c


Index: clang/test/CodeGen/debug-info-codeview-heapallocsite.c
===
--- clang/test/CodeGen/debug-info-codeview-heapallocsite.c
+++ clang/test/CodeGen/debug-info-codeview-heapallocsite.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -no-opaque-pointers -triple x86_64-windows-msvc 
-debug-info-kind=limited -gcodeview -fdeclspec -S -emit-llvm %s -o - | 
FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -debug-info-kind=limited 
-gcodeview -fdeclspec -S -emit-llvm %s -o - | FileCheck %s
 
 struct Foo;
 struct Bar;
@@ -14,10 +14,10 @@
 }
 
 // CHECK-LABEL: define {{.*}}void @call_alloc
-// CHECK: call i8* {{.*}}@alloc_void{{.*}} !heapallocsite [[DBG1:!.*]]
-// CHECK: call %struct.Foo* {{.*}}@alloc_foo{{.*}} !heapallocsite [[DBG2:!.*]]
-// CHECK: call i8* {{.*}}@alloc_void{{.*}} !heapallocsite [[DBG2]]
-// CHECK: call i8* {{.*}}@alloc_void{{.*}} !heapallocsite [[DBG3:!.*]]
+// CHECK: call ptr {{.*}}@alloc_void{{.*}} !heapallocsite [[DBG1:!.*]]
+// CHECK: call ptr {{.*}}@alloc_foo{{.*}} !heapallocsite [[DBG2:!.*]]
+// CHECK: call ptr {{.*}}@alloc_void{{.*}} !heapallocsite [[DBG2]]
+// CHECK: call ptr {{.*}}@alloc_void{{.*}} !heapallocsite [[DBG3:!.*]]
 
 // CHECK: [[DBG2]] = !DICompositeType(tag: DW_TAG_structure_type,
 // CHECK-SAME: name: "Foo"
Index: clang/lib/CodeGen/CGExprScalar.cpp
===
--- clang/lib/CodeGen/CGExprScalar.cpp
+++ clang/lib/CodeGen/CGExprScalar.cpp
@@ -2098,7 +2098,8 @@
 
 // Update heapallocsite metadata when there is an explicit pointer cast.
 if (auto *CI = dyn_cast(Src)) {
-  if (CI->getMetadata("heapallocsite") && isa(CE)) {
+  if (CI->getMetadata("heapallocsite") && isa(CE) &&
+  !isa(E)) {
 QualType PointeeType = DestTy->getPointeeType();
 if (!PointeeType.isNull())
   CGF.getDebugInfo()->addHeapAllocSiteMetadata(CI, PointeeType,


Index: clang/test/CodeGen/debug-info-codeview-heapallocsite.c
===
--- clang/test/CodeGen/debug-info-codeview-heapallocsite.c
+++ clang/test/CodeGen/debug-info-codeview-heapallocsite.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -no-opaque-pointers -triple x86_64-windows-msvc -debug-info-kind=limited -gcodeview -fdeclspec -S -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -debug-info-kind=limited -gcodeview -fdeclspec -S -emit-llvm %s -o - | FileCheck %s
 
 struct Foo;
 struct Bar;
@@ -14,10 +14,10 @@
 }
 
 // CHECK-LABEL: define {{.*}}void @call_alloc
-// CHECK: call i8* {{.*}}@alloc_void{{.*}} !heapallocsite [[DBG1:!.*]]
-// CHECK: call %struct.Foo* {{.*}}@alloc_foo{{.*}} !heapallocsite [[DBG2:!.*]]
-// CHECK: call i8* {{.*}}@alloc_void{{.*}} !heapallocsite [[DBG2]]
-// CHECK: call i8* {{.*}}@alloc_void{{.*}} !heapallocsite [[DBG3:!.*]]
+// CHECK: call ptr {{.*}}@alloc_void{{.*}} !heapallocsite [[DBG1:!.*]]
+// CHECK: call ptr {{.*}}@alloc_foo{{.*}} !heapallocsite [[DBG2:!.*]]
+// CHECK: call ptr {{.*}}@alloc_void{{.*}} !heapallocsite [[DBG2]]
+// CHECK: call ptr {{.*}}@alloc_void{{.*}} !heapallocsite [[DBG3:!.*]]
 
 // CHECK: [[DBG2]] = !DICompositeType(tag: DW_TAG_structure_type,
 // CHECK-SAME: name: "Foo"
Index: clang/lib/CodeGen/CGExprScalar.cpp
===
--- clang/lib/CodeGen/CGExprScalar.cpp
+++ clang/lib/CodeGen/CGExprScalar.cpp
@@ -2098,7 +2098,8 @@
 
 // Update heapallocsite metadata when there is an explicit pointer cast.
 if (auto *CI = dyn_cast(Src)) {
-  if (CI->getMetadata("heapallocsite") && isa(CE)) {
+  if (CI->getMetadata("heapallocsite") && isa(CE) &&
+  !isa(E)) {
 QualType PointeeType = DestTy->getPointeeType();
 if (!PointeeType.isNull())
   CGF.getDebugInfo()->addHeapAllocSiteMetadata(CI, PointeeType,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D149641: [docs] Hide collaboration and include graphs in doxygen docs

[Nikita Popov via Phabricator via cfe-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: StephenFan.

LGTM, only the inheritance graph is useful, which this preserves if I 
understand correctly (CLASS_GRAPH is still YES).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149641

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

[PATCH] D140538: [Clang][CodeGen] Use poison instead of undef for dummy values in CGExpr [NFC]

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

The demangling test shouldn't be modified at all.


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

https://reviews.llvm.org/D140538

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

[PATCH] D143624: Inlining: Run the legacy AlwaysInliner before the regular inliner.

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D143624#4316357 , @aeubanks wrote:

> In D143624#4315508 , @nikic wrote:
>
>> In D143624#4315468 , @dmgreen 
>> wrote:
>>
>>> It looks like there is quite a lot more optimization that happens to the 
>>> function being always-inlined (__SSAT) before this change. Through multiple 
>>> rounds of instcombine, almost to the end of the pass pipeline. The new 
>>> version runs a lot less before inlining, only running 
>>> instcombine->simplifycfg and not seeing another instcombine to clean up the 
>>> results. Is that because the AlwaysInlinePass is a module pass and it now 
>>> only runs the passes up to that point?
>>
>> Yes, which is why I personally think this change isn't a good idea. This 
>> essentially breaks our invariant that functions get simplified before they 
>> are inlined. This significantly alters the way alwaysinline functions will 
>> be optimized relative to normally inlined functions.
>
> That invariant shouldn't matter if we're not using heuristics to inline. The 
> normal heuristic-based inliner will still work on simplified callees, but now 
> with the additional benefit of seeing the state of an SCC where there may be 
> alwaysinline calls after the inlinings that must happen having happened.

I agree in the sense that for alwaysinling the simplification doesn't matter 
for cost modelling purposes. However, it can still have other benefits. One is 
that we don't repeat unnecessary work. Any simplification we do before inlining 
doesn't have to be repeated for every (potentially transitive) caller it gets 
inlined into. The other (and in a way, opposite) is that we simplify the 
function before inlining and then again the callee after inlining, which may 
paper over phase ordering issues (or the problem discussed above, which is kind 
of in the same category). Of course, this is not what this is intended for and 
we should endeavor to fix such issues -- but the practical outcome is still 
that you'll probably get worse optimization if you use alwaysinline vs inline 
after this change, which seems kind of unintuitive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143624

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

[PATCH] D143624: Inlining: Run the legacy AlwaysInliner before the regular inliner.

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

In D143624#4315468 , @dmgreen wrote:

> It looks like there is quite a lot more optimization that happens to the 
> function being always-inlined (__SSAT) before this change. Through multiple 
> rounds of instcombine, almost to the end of the pass pipeline. The new 
> version runs a lot less before inlining, only running 
> instcombine->simplifycfg and not seeing another instcombine to clean up the 
> results. Is that because the AlwaysInlinePass is a module pass and it now 
> only runs the passes up to that point?

Yes, which is why I personally think this change isn't a good idea. This 
essentially breaks our invariant that functions get simplified before they are 
inlined. This significantly alters the way alwaysinline functions will be 
optimized relative to normally inlined functions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143624

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

[PATCH] D149210: [IR] Change shufflevector undef mask to poison

[Nikita Popov via Phabricator via cfe-commits]

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

LGTM assuming tests pass.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149210

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

[PATCH] D148216: Add support for annotations in UpdateTestChecks (NFC)

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: 
clang/test/utils/update_cc_test_checks/Inputs/annotations.c.expected:12
+// CHECK-NEXT:[[TMP1:%.*]] = load i32, ptr [[X]], align 4
+// CHECK-NEXT:ret i32 [[TMP1]]
+//

hnrklssn wrote:
> delcypher wrote:
> > @hnrklssn I just noticed we don't have a `CHECK` for what `META2` actually 
> > refers to. Should we?
> > 
> > Not something that has to be fixed in this patch, more just an observation.
> Indeed this is true for metadata in general, presumably because the RHS often 
> refer to things like other metadata identifiers. In the case of annotations 
> they seem to always refer to simple strings however, so it would be feasible 
> to do a straight match without having to do recursive matching or complex 
> regexes to determine which part of the metadata to match against.
> 
> In many cases with metadata attached to IR nodes, multiple nodes refer to the 
> same metadata node, so at least you verify that they still are consistent. 
> But I agree that verifying the content would be a great future addition.
You need to pass `--check-globals` to check the actual metadata.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148216

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