[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added inline comments.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:1541
+CGF.Builder.CreateBr(CastFail);
+return llvm::UndefValue::get(CGF.VoidPtrTy);
+  }

Please use PoisonValue as a placeholder whenever possible. We are trying to get 
rid of undef.
Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154658

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

[PATCH] D147732: [AMDGPU] Add type mangling for {read, write, readfirst, perm}lane intrinsics

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added inline comments.



Comment at: llvm/lib/Target/AMDGPU/AMDGPULateCodeGenPrepare.cpp:213
+
+Value *Result = UndefValue::get(Ty);
+for (int i = 0; i < EC; i += 1 + is16Bit) {

Please use poison wherever possible. In this case it seems it's just a 
placeholder, so it can be poison.
We're trying to get rid of poison. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147732

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

[PATCH] D149548: [IR] Update to use new shufflevector semantics

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D149548#4413598 , @uweigand wrote:

> So the semantics of the `vec_promote(a, b)` intrinsic is defined as:
>
>> Returns a vector with a in element position b. The result is a vector with a 
>> in element position b. [...] The other elements of the vector are undefined.
>
> This is currently implemented by using `insertvector` to place `a` at 
> position `b` into a source vector that is `undef`.   The effect should be 
> that when using element `b` of that vector, we are guaranteed to get `a`, 
> while using any other element is undefined behavior (just like accessing an 
> uninitialized variable).
>
> To be honest, I'm not sure how exactly the LLVM IR semantics changes here 
> when using a `poison` source vector instead of `undef`.  I seem to recall 
> that `poison` propagates over operations - is it true that the result of 
> `insertvector` on a `poison` vector is itself `poison`?  If so, then this 
> change would break semantics.   However, if the result is a vector with `a` 
> in element `b`, and `poison` only in the other elements, then I guess this 
> would still preserve the expected semantics.

If a vector is fully initialized with `insertvector` (i.e., one operation per 
index), then the value of the base vector is irrelevant. It can be poison.
Poison in vectors is element-wise.  doesn't propagate to .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149548

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

[PATCH] D152658: [InstCombine] Change SimplifyDemandedVectorElts to use PoisonElts instead of UndefElts

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added inline comments.



Comment at: llvm/lib/Target/X86/X86InstCombineIntrinsic.cpp:3102-3105
   case Intrinsic::x86_sse4a_extrqi:
   case Intrinsic::x86_sse4a_insertq:
   case Intrinsic::x86_sse4a_insertqi:
+PoisonElts.setHighBits(VWidth / 2);

this change doesn't look correct. The bits are undefined, doesn't seem like you 
can use poison here.



Comment at: llvm/test/Transforms/InstCombine/vec_shuffle.ll:1301
 ; CHECK-LABEL: @fmul_splat_constant(
-; CHECK-NEXT:[[TMP1:%.*]] = fmul <2 x float> [[X:%.*]], 
-; CHECK-NEXT:[[R:%.*]] = shufflevector <2 x float> [[TMP1]], <2 x float> 
poison, <2 x i32> zeroinitializer
+  ; CHECK-NEXT:[[TMP1:%.*]] = shufflevector <2 x float> [[X:%.*]], <2 x 
float> undef, <2 x i32> zeroinitializer
+; CHECK-NEXT:[[R:%.*]] = fmul <2 x float> [[TMP1]], 

Why does it regress here (goes from poison to undef)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152658

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

[PATCH] D148654: Modify BoundsSan to improve debuggability

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added inline comments.



Comment at: llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp:189
   auto GetTrapBB = [](BuilderTy ) {
-if (TrapBB && SingleTrapBB)
-  return TrapBB;
-
-Function *Fn = IRB.GetInsertBlock()->getParent();
-// FIXME: This debug location doesn't make a lot of sense in the
-// `SingleTrapBB` case.
-auto DebugLoc = IRB.getCurrentDebugLocation();
-IRBuilder<>::InsertPointGuard Guard(IRB);
-TrapBB = BasicBlock::Create(Fn->getContext(), "trap", Fn);
-IRB.SetInsertPoint(TrapBB);
-
-auto *F = Intrinsic::getDeclaration(Fn->getParent(), Intrinsic::trap);
-CallInst *TrapCall = IRB.CreateCall(F, {});
-TrapCall->setDoesNotReturn();
-TrapCall->setDoesNotThrow();
-TrapCall->setDebugLoc(DebugLoc);
-IRB.CreateUnreachable();
-
+if (DebugTrapBB) {
+  Function *Fn = IRB.GetInsertBlock()->getParent();

this seems like code duplication. This pass already has the single-trap flag to 
exactly control if you get a single trap BB or one per check for better debug 
info.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148654

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

[PATCH] D143287: [Clang][X86] Change X86 cast intrinsics to use __builtin_nondeterministic_value

[Nuno Lopes via Phabricator via cfe-commits]

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

LGTM, but please wait for another reviewer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143287

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

[PATCH] D147572: [Clang][OpenMP] Fix failure with team-wide allocated variable

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:3355
+llvm::GlobalValue::InternalLinkage,
+CGM.getTriple().isAMDGCN() ? llvm::UndefValue::get(VarTy)
+   : llvm::Constant::getNullValue(VarTy),

Please use poison instead of undef wherever possible as we are tying to remove 
undef. The replacement is usually safe when you just need a placeholder.
Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147572

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

[PATCH] D143287: [Clang][X86] Change X86 cast intrinsics to use __builtin_nondeterministic_value

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D143287#4202174 , @ManuelJBrito 
wrote:

> Implementing the 128 to 512 casts by filling the rest of the vector with the 
> same definition of a nondeterministic_value is not correct because  :
>
> a = freeze poison
> v = 
>
> is not the same as
>
> v = freeze poison
>
> The only solution I'm seeing ,using the shufflevector, is doing the 
> conversion in two steps:
>
> - build a 256 vector with the upper half being undefined( freeze poison)
> - build a 512 vector where the lower half is the previous 256 vector and the 
> upper half being undefined
>
> I think this would require two shuffles which is unfortunate.
>
> This would ensure no miscompilations due to multiple uses of the same freeze 
> undef/poison but would probably require some backend work to ensure the 
> pattern is recognized to emit efficient assembly.

Semantically that is correct.
But the backend may require some tweaks to recognize this pattern.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143287

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

[PATCH] D144903: [X86] Drop single use check for freeze(undef) in LowerAVXCONCAT_VECTORS

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D144903#4182498 , @efriedma wrote:

> I think the point of the hasOneUse check is to avoid a possible miscompile; 
> if a FREEZE has more than one use, all users need to see the same value.  So 
> not sure dropping the check is correct in general.

Good point.
This patch is not correct, must be reverted.
The shufflevectors create copies of a same 'freeze poison'. These copies must 
see the same value, and that's not the case after this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144903

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

[PATCH] D144590: Fix shared memory allocation on AMDGPU

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

Please use poison values as place holders instead of undef values as we're 
trying to get rid of Undef. Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144590

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

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

The issue is the function call, not the stores.
Stores are valid as long as they are to local memory (stack or allocated by the 
function).

Because of the call, you can't tag it as `memory(none)`. But you may be able to 
tag it with inaccessiblemem if the call is also marked as such. Otherwise, no.

Performance wise it shouldn't matter much. How many functions are tagged with 
pure/const in source code? So here I would lean towards correctness.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138958

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

[PATCH] D134410: [clang][CodeGen] Add noundef metadata to load instructions (preliminary 1 or 5)

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D134410#3983684 , @nikic wrote:

> The other problem I see here is that we still need to do something about the 
> memcpy -> load/store fold. As soon as we have poison from uninit values 
> (either directly or via `!uninit_is_poison`) this will start causing 
> miscompiles very quickly. The only way to do this right now is again with an 
> `` vector load/store, which still optimizes terribly. This needs 
> either load+store of integer to preserve poison, or again some form of byte 
> type.
>
> Something I've only recently realized is that we also run into this problem 
> when inserting spurious load/store pairs, e.g. as done by LICM scalar 
> promotion. If we're promoting say an i32 value to scalar that previously used 
> a conditional store, then promotion will now introduce an unconditional load 
> and store, which will spread poison from individual bytes. So that means that 
> technically scalar promotion (and SimplifyCFG store speculation) also need to 
> do some special dance to preserve poison. And without preservation of poison 
> across load/store/phi in plain ints, this is going to be a bad optimization 
> outcome either way: We'd have to use either a vector of i8 or a byte type for 
> the inserted phi nodes and only cast to integer and back when manipulating 
> the value, which would probably defeat the optimization. At that point 
> probably best to freeze the first load (which again needs a freezing load).

For this stuff, I think the ideal solution is the byte type. That's the only 
way to do this kind of raw byte copies. Doesn't spread poison between bits nor 
do you need to know if you are reading a pointer or something else. Nor does it 
require a freeze, which is annoying.

John will submit a proposal (next week?) using poison for uninit loads. I think 
we have converged on something cool. Thanks for insisting on that one!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134410

___
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]

[Nuno Lopes via Phabricator via cfe-commits]

nlopes accepted this revision.
nlopes added a comment.

LGTM, thank you!


Repository:
  rG LLVM Github Monorepo

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] D139745: [Clang]Use poison instead of undef where its used as placeholder[NFC]

[Nuno Lopes 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 rG2482dbff461e: [Clang] Use poison instead of undef where its 
used as placeholder [NFC] (authored by ManuelJBrito, committed by nlopes).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139745

Files:
  clang/lib/CodeGen/CGException.cpp
  clang/lib/CodeGen/CGExprScalar.cpp
  clang/test/CodeGen/builtinshufflevector2.c
  clang/test/CodeGenCXX/nrvo.cpp
  clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp
  clang/test/CodeGenCXX/vla-consruct.cpp
  clang/test/CodeGenCoroutines/coro-eh-cleanup-exp-namespace.cpp
  clang/test/CodeGenCoroutines/coro-eh-cleanup.cpp
  clang/test/OpenMP/distribute_parallel_for_num_threads_codegen.cpp
  clang/test/OpenMP/distribute_parallel_for_simd_num_threads_codegen.cpp
  clang/test/OpenMP/single_codegen.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_num_threads_codegen.cpp
  clang/test/OpenMP/teams_distribute_parallel_for_simd_num_threads_codegen.cpp
  clang/test/OpenMP/threadprivate_codegen.cpp

Index: clang/test/OpenMP/threadprivate_codegen.cpp
===
--- clang/test/OpenMP/threadprivate_codegen.cpp
+++ clang/test/OpenMP/threadprivate_codegen.cpp
@@ -1123,7 +1123,7 @@
 // CHECK1:   eh.resume:
 // CHECK1-NEXT:[[EXN:%.*]] = load ptr, ptr [[EXN_SLOT]], align 8
 // CHECK1-NEXT:[[SEL:%.*]] = load i32, ptr [[EHSELECTOR_SLOT]], align 4
-// CHECK1-NEXT:[[LPAD_VAL:%.*]] = insertvalue { ptr, i32 } undef, ptr [[EXN]], 0
+// CHECK1-NEXT:[[LPAD_VAL:%.*]] = insertvalue { ptr, i32 } poison, ptr [[EXN]], 0
 // CHECK1-NEXT:[[LPAD_VAL28:%.*]] = insertvalue { ptr, i32 } [[LPAD_VAL]], i32 [[SEL]], 1
 // CHECK1-NEXT:resume { ptr, i32 } [[LPAD_VAL28]]
 //
@@ -1331,7 +1331,7 @@
 // CHECK1:   eh.resume:
 // CHECK1-NEXT:[[EXN:%.*]] = load ptr, ptr [[EXN_SLOT]], align 8
 // CHECK1-NEXT:[[SEL:%.*]] = load i32, ptr [[EHSELECTOR_SLOT]], align 4
-// CHECK1-NEXT:[[LPAD_VAL:%.*]] = insertvalue { ptr, i32 } undef, ptr [[EXN]], 0
+// CHECK1-NEXT:[[LPAD_VAL:%.*]] = insertvalue { ptr, i32 } poison, ptr [[EXN]], 0
 // CHECK1-NEXT:[[LPAD_VAL22:%.*]] = insertvalue { ptr, i32 } [[LPAD_VAL]], i32 [[SEL]], 1
 // CHECK1-NEXT:resume { ptr, i32 } [[LPAD_VAL22]]
 //
@@ -1446,7 +1446,7 @@
 // CHECK1:   eh.resume:
 // CHECK1-NEXT:[[EXN:%.*]] = load ptr, ptr [[EXN_SLOT]], align 8
 // CHECK1-NEXT:[[SEL:%.*]] = load i32, ptr [[EHSELECTOR_SLOT]], align 4
-// CHECK1-NEXT:[[LPAD_VAL:%.*]] = insertvalue { ptr, i32 } undef, ptr [[EXN]], 0
+// CHECK1-NEXT:[[LPAD_VAL:%.*]] = insertvalue { ptr, i32 } poison, ptr [[EXN]], 0
 // CHECK1-NEXT:[[LPAD_VAL15:%.*]] = insertvalue { ptr, i32 } [[LPAD_VAL]], i32 [[SEL]], 1
 // CHECK1-NEXT:resume { ptr, i32 } [[LPAD_VAL15]]
 //
@@ -1854,7 +1854,7 @@
 // CHECK2:   eh.resume:
 // CHECK2-NEXT:[[EXN:%.*]] = load ptr, ptr [[EXN_SLOT]], align 8
 // CHECK2-NEXT:[[SEL:%.*]] = load i32, ptr [[EHSELECTOR_SLOT]], align 4
-// CHECK2-NEXT:[[LPAD_VAL:%.*]] = insertvalue { ptr, i32 } undef, ptr [[EXN]], 0
+// CHECK2-NEXT:[[LPAD_VAL:%.*]] = insertvalue { ptr, i32 } poison, ptr [[EXN]], 0
 // CHECK2-NEXT:[[LPAD_VAL22:%.*]] = insertvalue { ptr, i32 } [[LPAD_VAL]], i32 [[SEL]], 1
 // CHECK2-NEXT:resume { ptr, i32 } [[LPAD_VAL22]]
 //
@@ -1975,7 +1975,7 @@
 // CHECK2:   eh.resume:
 // CHECK2-NEXT:[[EXN:%.*]] = load ptr, ptr [[EXN_SLOT]], align 8
 // CHECK2-NEXT:[[SEL:%.*]] = load i32, ptr [[EHSELECTOR_SLOT]], align 4
-// CHECK2-NEXT:[[LPAD_VAL:%.*]] = insertvalue { ptr, i32 } undef, ptr [[EXN]], 0
+// CHECK2-NEXT:[[LPAD_VAL:%.*]] = insertvalue { ptr, i32 } poison, ptr [[EXN]], 0
 // CHECK2-NEXT:[[LPAD_VAL28:%.*]] = insertvalue { ptr, i32 } [[LPAD_VAL]], i32 [[SEL]], 1
 // CHECK2-NEXT:resume { ptr, i32 } [[LPAD_VAL28]]
 //
@@ -2100,7 +2100,7 @@
 // CHECK2:   eh.resume:
 // CHECK2-NEXT:[[EXN:%.*]] = load ptr, ptr [[EXN_SLOT]], align 8
 // CHECK2-NEXT:[[SEL:%.*]] = load i32, ptr [[EHSELECTOR_SLOT]], align 4
-// CHECK2-NEXT:[[LPAD_VAL:%.*]] = insertvalue { ptr, i32 } undef, ptr [[EXN]], 0
+// CHECK2-NEXT:[[LPAD_VAL:%.*]] = insertvalue { ptr, i32 } poison, ptr [[EXN]], 0
 // CHECK2-NEXT:[[LPAD_VAL15:%.*]] = insertvalue { ptr, i32 } [[LPAD_VAL]], i32 [[SEL]], 1
 // CHECK2-NEXT:resume { ptr, i32 } [[LPAD_VAL15]]
 //
@@ -2529,7 +2529,7 @@
 // SIMD1:   eh.resume:
 // SIMD1-NEXT:[[EXN:%.*]] = load ptr, ptr [[EXN_SLOT]], align 8
 // SIMD1-NEXT:[[SEL:%.*]] = load i32, ptr [[EHSELECTOR_SLOT]], align 4
-// SIMD1-NEXT:[[LPAD_VAL:%.*]] = insertvalue { ptr, i32 } undef, ptr [[EXN]], 0
+// SIMD1-NEXT:[[LPAD_VAL:%.*]] = insertvalue { ptr, i32 } poison, ptr [[EXN]], 0
 // SIMD1-NEXT:[[LPAD_VAL22:%.*]] = insertvalue { ptr, i32 } [[LPAD_VAL]], 

[PATCH] D139745: [Clang]Use poison instead of undef where its used as placeholder[NFC]

[Nuno Lopes via Phabricator via cfe-commits]

nlopes accepted this revision.
nlopes 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/D139745/new/

https://reviews.llvm.org/D139745

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

[PATCH] D138755: [Clang][CodeGen] Use poison instead of undef for extra argument in __builtin_amdgcn_mov_dpp [NFC]

[Nuno Lopes 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 rG481170cb5511: [Clang][CodeGen] Use poison instead of undef 
for extra argument in… (authored by ManuelJBrito, committed by nlopes).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138755

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGenOpenCL/builtins-amdgcn-vi.cl


Index: clang/test/CodeGenOpenCL/builtins-amdgcn-vi.cl
===
--- clang/test/CodeGenOpenCL/builtins-amdgcn-vi.cl
+++ clang/test/CodeGenOpenCL/builtins-amdgcn-vi.cl
@@ -101,7 +101,7 @@
 }
 
 // CHECK-LABEL: @test_mov_dpp
-// CHECK: call i32 @llvm.amdgcn.update.dpp.i32(i32 undef, i32 %src, i32 0, i32 
0, i32 0, i1 false)
+// CHECK: call i32 @llvm.amdgcn.update.dpp.i32(i32 poison, i32 %src, i32 0, 
i32 0, i32 0, i1 false)
 void test_mov_dpp(global int* out, int src)
 {
   *out = __builtin_amdgcn_mov_dpp(src, 0, 0, 0, false);
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -16905,7 +16905,7 @@
   Args.push_back(EmitScalarExpr(E->getArg(I)));
 assert(Args.size() == 5 || Args.size() == 6);
 if (Args.size() == 5)
-  Args.insert(Args.begin(), llvm::UndefValue::get(Args[0]->getType()));
+  Args.insert(Args.begin(), llvm::PoisonValue::get(Args[0]->getType()));
 Function *F =
 CGM.getIntrinsic(Intrinsic::amdgcn_update_dpp, Args[0]->getType());
 return Builder.CreateCall(F, Args);


Index: clang/test/CodeGenOpenCL/builtins-amdgcn-vi.cl
===
--- clang/test/CodeGenOpenCL/builtins-amdgcn-vi.cl
+++ clang/test/CodeGenOpenCL/builtins-amdgcn-vi.cl
@@ -101,7 +101,7 @@
 }
 
 // CHECK-LABEL: @test_mov_dpp
-// CHECK: call i32 @llvm.amdgcn.update.dpp.i32(i32 undef, i32 %src, i32 0, i32 0, i32 0, i1 false)
+// CHECK: call i32 @llvm.amdgcn.update.dpp.i32(i32 poison, i32 %src, i32 0, i32 0, i32 0, i1 false)
 void test_mov_dpp(global int* out, int src)
 {
   *out = __builtin_amdgcn_mov_dpp(src, 0, 0, 0, false);
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -16905,7 +16905,7 @@
   Args.push_back(EmitScalarExpr(E->getArg(I)));
 assert(Args.size() == 5 || Args.size() == 6);
 if (Args.size() == 5)
-  Args.insert(Args.begin(), llvm::UndefValue::get(Args[0]->getType()));
+  Args.insert(Args.begin(), llvm::PoisonValue::get(Args[0]->getType()));
 Function *F =
 CGM.getIntrinsic(Intrinsic::amdgcn_update_dpp, Args[0]->getType());
 return Builder.CreateCall(F, Args);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D134410: [clang][CodeGen] Add noundef metadata to load instructions (preliminary 1 or 2)

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D134410#3898563 , @nikic wrote:

> In D134410#3895253 , @nlopes wrote:
>
>> In D134410#3895077 , @nikic wrote:
>>
>>> In D134410#3894995 , @nlopes 
>>> wrote:
>>>
 We wanted this patch to make us switch uninitialized loads to poison at 
 will, since they become UB. In practice, this helps us fixing bugs in SROA 
 and etc without perf degradation.
>>>
>>> Can you elaborate on this? I don't see how this is necessary for switching 
>>> uninitialized loads to poison.
>>
>> It's not mandatory, it's a simple way of achieving it as !noundef already 
>> exists.
>>
>> We cannot change the default behavior of load as it would break BC.
>
> FWIW, I don't agree with this. It's fine to change load semantics, as long as 
> bitcode autoupgrade is handled correctly. I'd go even further and say that at 
> least long term, any solution that does not have a plain `load` instruction 
> return `poison` for uninitialized memory will be a maintenance mess.
>
> I think the core problem that prevents us from moving in this direction is 
> that we have no way to represent a frozen load at all (as `freeze(load)` will 
> propagate poison before freezing). If I understand correctly, you're trying 
> to solve this by making *all* loads frozen loads, and use `load !noundef` to 
> allow dropping the freeze. I think this would be a very bad outcome, 
> especially as we're going to lose that `!noundef` on most load speculations, 
> and I don't think we want to be introducing freezes when speculating loads 
> (e.g. during LICM).
>
> I expect that the path of least resistance is going to be to support bitwise 
> poison for load/store/phi/select and promote it to full-value poison on any 
> other operation, allowing a freezing load to be expressed as `freeze(load)`.
>
> Please let me know if I completely misunderstood the scheme you have in 
> mind...

You got it right. But the load we propose is not exactly a freezing load. It 
only returns `freeze poison` for uninit memory. It doesn't freeze stored values.
If we introduce a `!uninit_is_poison` flag, we can drop !noundef when hoisting 
and add `!uninit_is_poison` instead (it's implied by !noundef).

The question is what's the default for uninit memory: `freeze poison` or 
`poison`? But I think we need both. And since we need both (unless we add a 
freezing load or bitwise poison), why not keep a behavior closer to the current?
A freezing load is worse as a store+load needs to be forwarded though a freeze, 
as the load is not a NOP anymore.

Bitwise poison would be nice, but I don't know how to make it work. To make it 
work with bit-fields, we would need and/or to propagate poison bit-wise as 
well. But then we will break optimizations that transform between those and 
arithmetic. Then you start wanting that add/mul/etc also propagate poison 
bit-wise and then I don't know how to specify that semantics.  (if you want, I 
can forward you a proposal we wrote a few years ago, but we never managed to 
make sound, so it was never presented in public)

I agree that bit-wise poison for loads sounds appealing (for bit fields, load 
widening, etc), but without support in the rest of the IR, it's not worth much. 
If it becomes value-wise in the 1st operation, then I don't think we gain any 
expressivity.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134410

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

[PATCH] D134410: [clang][CodeGen] Add noundef metadata to load instructions (preliminary 1 or 2)

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D134410#3895077 , @nikic wrote:

> In D134410#3894995 , @nlopes wrote:
>
>> We wanted this patch to make us switch uninitialized loads to poison at 
>> will, since they become UB. In practice, this helps us fixing bugs in SROA 
>> and etc without perf degradation.
>
> Can you elaborate on this? I don't see how this is necessary for switching 
> uninitialized loads to poison.

It's not mandatory, it's a simple way of achieving it as !noundef already 
exists.

We cannot change the default behavior of load as it would break BC. An 
alternative is to introduce a new !poison_on_unint for loads. Clang could use 
that on all loads except those for bit-fields.
Our suggestion is to jump straight to !noundef.
To fully remove undef, then we need to swtich loads to return `freeze poison` 
rather than undef on uninitialized access, but only after we are able to yield 
poison in the common case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134410

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

[PATCH] D134410: [clang][CodeGen] Add noundef metadata to load instructions (preliminary 1 or 2)

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D134410#3893918 , @nikic wrote:

> I think adding this under a default-disabled flag is fine for evaluation 
> purposes, but I have doubts that we will ever be able to enable this by 
> default. There is a lot of code out there assuming that copying uninitialized 
> data around is fine.

Well, I think that flags that are disabled by default don't exist and they are 
not useful.
We wanted this patch to make us switch uninitialized loads to poison at will, 
since they become UB. In practice, this helps us fixing bugs in SROA and etc 
without perf degradation.
As long as ubsan/valgrind can detect these uninitialized loads, I think we 
should be ok to deploy this change. We are touching memcpys yet, at least, as 
those may be susceptible to handling uninit memory.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134410

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

[PATCH] D136998: Try to implement lambdas with inalloca parameters by inlining the call operator function.

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added inline comments.



Comment at: clang/lib/CodeGen/CGClass.cpp:3080
+  if (I.getName().equals("this")) {
+VMap[] = llvm::UndefValue::get(I.getType());
+  }

Please use PoisonValue instead whenever possible, as we are trying to remove 
undef from LLVM.
Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136998

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

[PATCH] D136284: SROA should freeze undefs for loads with no prior stores

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D136284#3874755 , @jamieschmeiser 
wrote:

> In D136284#3874614 , @nlopes wrote:
>
>> In D136284#3874596 , 
>> @jamieschmeiser wrote:
>>
>>> In D136284#3874492 , @nikic wrote:
>>>
 At least in C++, working with uninitialized memory is pretty much always 
 immediate undefined behavior, see https://eel.is/c++draft/basic.indet for 
 the relevant wording. The only exception are "copy-like" operations on 
 unsigned character types, which comparisons do not fall under.

 I believe the C specification is less clear cut about this, but Clang and 
 LLVM assume basically the same to also hold for C code.
>>>
>>> What version of the C++ standard is this?  Every version that I have seen 
>>> has basics as section 3 and I cannot find this section, nor anything 
>>> similar.  Section 6 is Statements.
>>
>> That discussion is orthogonal to this patch.
>> This patch is not desired because it's not needed per the current LLVM IR 
>> semantics. If you want to change something, you need to start by proposing a 
>> change to the LLVM IR semantics. You'll need to justify why it's needed, why 
>> it's correct, the perf impact, how to make it backwards compatible, why it's 
>> better than the proposals over the table right now.
>>
>> Anyway, a patch like this solves no problem. LLVM allows loads to be 
>> duplicated. Your patch does nothing to prevent that and to ensure all loads 
>> see the same value. The issue is way more complicated than what this patch 
>> implies.
>
> I'm not trying to flog a dead horse (I've already abandoned this) but I am 
> trying to understand this statement.  I do not dispute that there may be 
> other situations similar to this but, assuming that we did want to ensure 
> that the loads had the same value, why is freezing them at this point not the 
> correct thing to do?  Whether they are poison or undef, freezing them would 
> ensure that they compare equal.  Yes, I understand it may have performance 
> impacts, there may be better ways, etc.  But, ignoring all that, isn't this 
> exactly what freeze is designed for?

It's not enough to ensure the semantics you want. What about optimizations that 
happen before and after SROA? This patch only deals with a subset of the cases 
(the ones that are detected by SROA's algorithm). Again, load duplication 
happens, and this patch doesn't deal with it. So it's inconsistent.
To implement the proposed semantics, you would need to change quite a few 
optimizations, not just SROA.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136284

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

[PATCH] D136284: SROA should freeze undefs for loads with no prior stores

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D136284#3874596 , @jamieschmeiser 
wrote:

> In D136284#3874492 , @nikic wrote:
>
>> At least in C++, working with uninitialized memory is pretty much always 
>> immediate undefined behavior, see https://eel.is/c++draft/basic.indet for 
>> the relevant wording. The only exception are "copy-like" operations on 
>> unsigned character types, which comparisons do not fall under.
>>
>> I believe the C specification is less clear cut about this, but Clang and 
>> LLVM assume basically the same to also hold for C code.
>
> What version of the C++ standard is this?  Every version that I have seen has 
> basics as section 3 and I cannot find this section, nor anything similar.  
> Section 6 is Statements.

That discussion is orthogonal to this patch.
This patch is not desired because it's not needed per the current LLVM IR 
semantics. If you want to change something, you need to start by proposing a 
change to the LLVM IR semantics. You'll need to justify why it's needed, why 
it's correct, the perf impact, how to make it backwards compatible, why it's 
better than the proposals over the table right now.

Anyway, a patch like this solves no problem. LLVM allows loads to be 
duplicated. Your patch does nothing to prevent that and to ensure all loads see 
the same value. The issue is way more complicated than what this patch implies.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136284

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

[PATCH] D136284: SROA should freeze undefs for loads with no prior stores

[Nuno Lopes via Phabricator via cfe-commits]

nlopes requested changes to this revision.
nlopes added a comment.

> However, multiple loads of the same memory must be equal

This premise is incorrect w.r.t. with current semantics, which say that loads 
return undef for uninit memory.

As Nikita mentioned we are working towards changing this semantics. The work is 
ongoing, but we pivoted to make a couple of improvements in other areas to 
avoid perf regressions. For example, see the clang !noundef patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136284

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

[PATCH] D135091: Create storage for the `_cmd` argument to the helper function for generated getters/setters of `direct` Objective-C properties.

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added inline comments.



Comment at: clang/lib/CodeGen/CGObjC.cpp:1125
+llvm::Type *selType = CGF.ConvertType(CGF.getContext().getObjCSelType());
+return llvm::UndefValue::get(selType);
+  }

mwyman wrote:
> nlopes wrote:
> > Please consider using PoisonValue here instead (if appropriate). We are 
> > trying to get rid of undef.
> > Thank you!
> > A poison value is a result of an erroneous operation.
> 
> I could very well be misunderstanding, but based on this description from 
> LangRef.html `PoisonValue` doesn't seem appropriate here; this is not 
> "erroneous" behavior: it is just continuing the behavior prior to removing 
> the `_cmd` implicit argument from the ABI, which was that the value was 
> `undef` from the generated getter/setter's caller.
> 
> https://github.com/llvm/llvm-project/blob/49acab3f1408be9439a57833aa7b67678c9a05ba/clang/lib/CodeGen/CGObjCMac.cpp#L2142
> 
> Since this is (essentially) replicating that behavior, I'm not sure 
> `PoisonValue` is what we want.
If the value is not used, then it can be poison.
If it gets used somehow then it depends on the callers. I don't know anything 
about ObjC to know what the right answer is here.
As we want to remove undef, the fewer the uses the better. Thank you!


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

https://reviews.llvm.org/D135091

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

[PATCH] D135392: Use PoisonValue in vector BIs [NFC]

[Nuno Lopes via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG14e2592ff611: [clang][CodeGen] Use poison instead of undef 
as placeholder in ARM builtins… (authored by ManuelJBrito, committed by nlopes).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135392

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/PowerPC/builtins-ppc-int128.c
  clang/test/CodeGen/aarch64-bf16-ldst-intrinsics.c
  clang/test/CodeGen/aarch64-neon-intrinsics.c
  clang/test/CodeGen/aarch64-neon-ldst-one.c
  clang/test/CodeGen/aarch64-neon-scalar-x-indexed-elem.c
  clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_dupq-bfloat.c
  clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_dupq.c
  clang/test/CodeGen/arm-mve-intrinsics/vld24.c
  clang/test/CodeGen/arm_neon_intrinsics.c

Index: clang/test/CodeGen/arm_neon_intrinsics.c
===
--- clang/test/CodeGen/arm_neon_intrinsics.c
+++ clang/test/CodeGen/arm_neon_intrinsics.c
@@ -4077,7 +4077,7 @@
 
 // CHECK-LABEL: @test_vld1q_dup_u8(
 // CHECK:   [[TMP0:%.*]] = load i8, i8* %a, align 1
-// CHECK:   [[TMP1:%.*]] = insertelement <16 x i8> undef, i8 [[TMP0]], i32 0
+// CHECK:   [[TMP1:%.*]] = insertelement <16 x i8> poison, i8 [[TMP0]], i32 0
 // CHECK:   [[LANE:%.*]] = shufflevector <16 x i8> [[TMP1]], <16 x i8> [[TMP1]], <16 x i32> zeroinitializer
 // CHECK:   ret <16 x i8> [[LANE]]
 uint8x16_t test_vld1q_dup_u8(uint8_t const * a) {
@@ -4088,7 +4088,7 @@
 // CHECK:   [[TMP0:%.*]] = bitcast i16* %a to i8*
 // CHECK:   [[TMP1:%.*]] = bitcast i8* [[TMP0]] to i16*
 // CHECK:   [[TMP2:%.*]] = load i16, i16* [[TMP1]], align 2
-// CHECK:   [[TMP3:%.*]] = insertelement <8 x i16> undef, i16 [[TMP2]], i32 0
+// CHECK:   [[TMP3:%.*]] = insertelement <8 x i16> poison, i16 [[TMP2]], i32 0
 // CHECK:   [[LANE:%.*]] = shufflevector <8 x i16> [[TMP3]], <8 x i16> [[TMP3]], <8 x i32> zeroinitializer
 // CHECK:   ret <8 x i16> [[LANE]]
 uint16x8_t test_vld1q_dup_u16(uint16_t const * a) {
@@ -4099,7 +4099,7 @@
 // CHECK:   [[TMP0:%.*]] = bitcast i32* %a to i8*
 // CHECK:   [[TMP1:%.*]] = bitcast i8* [[TMP0]] to i32*
 // CHECK:   [[TMP2:%.*]] = load i32, i32* [[TMP1]], align 4
-// CHECK:   [[TMP3:%.*]] = insertelement <4 x i32> undef, i32 [[TMP2]], i32 0
+// CHECK:   [[TMP3:%.*]] = insertelement <4 x i32> poison, i32 [[TMP2]], i32 0
 // CHECK:   [[LANE:%.*]] = shufflevector <4 x i32> [[TMP3]], <4 x i32> [[TMP3]], <4 x i32> zeroinitializer
 // CHECK:   ret <4 x i32> [[LANE]]
 uint32x4_t test_vld1q_dup_u32(uint32_t const * a) {
@@ -4110,7 +4110,7 @@
 // CHECK:   [[TMP0:%.*]] = bitcast i64* %a to i8*
 // CHECK:   [[TMP1:%.*]] = bitcast i8* [[TMP0]] to i64*
 // CHECK:   [[TMP2:%.*]] = load i64, i64* [[TMP1]], align 4
-// CHECK:   [[TMP3:%.*]] = insertelement <2 x i64> undef, i64 [[TMP2]], i32 0
+// CHECK:   [[TMP3:%.*]] = insertelement <2 x i64> poison, i64 [[TMP2]], i32 0
 // CHECK:   [[LANE:%.*]] = shufflevector <2 x i64> [[TMP3]], <2 x i64> [[TMP3]], <2 x i32> zeroinitializer
 // CHECK:   ret <2 x i64> [[LANE]]
 uint64x2_t test_vld1q_dup_u64(uint64_t const * a) {
@@ -4119,7 +4119,7 @@
 
 // CHECK-LABEL: @test_vld1q_dup_s8(
 // CHECK:   [[TMP0:%.*]] = load i8, i8* %a, align 1
-// CHECK:   [[TMP1:%.*]] = insertelement <16 x i8> undef, i8 [[TMP0]], i32 0
+// CHECK:   [[TMP1:%.*]] = insertelement <16 x i8> poison, i8 [[TMP0]], i32 0
 // CHECK:   [[LANE:%.*]] = shufflevector <16 x i8> [[TMP1]], <16 x i8> [[TMP1]], <16 x i32> zeroinitializer
 // CHECK:   ret <16 x i8> [[LANE]]
 int8x16_t test_vld1q_dup_s8(int8_t const * a) {
@@ -4130,7 +4130,7 @@
 // CHECK:   [[TMP0:%.*]] = bitcast i16* %a to i8*
 // CHECK:   [[TMP1:%.*]] = bitcast i8* [[TMP0]] to i16*
 // CHECK:   [[TMP2:%.*]] = load i16, i16* [[TMP1]], align 2
-// CHECK:   [[TMP3:%.*]] = insertelement <8 x i16> undef, i16 [[TMP2]], i32 0
+// CHECK:   [[TMP3:%.*]] = insertelement <8 x i16> poison, i16 [[TMP2]], i32 0
 // CHECK:   [[LANE:%.*]] = shufflevector <8 x i16> [[TMP3]], <8 x i16> [[TMP3]], <8 x i32> zeroinitializer
 // CHECK:   ret <8 x i16> [[LANE]]
 int16x8_t test_vld1q_dup_s16(int16_t const * a) {
@@ -4141,7 +4141,7 @@
 // CHECK:   [[TMP0:%.*]] = bitcast i32* %a to i8*
 // CHECK:   [[TMP1:%.*]] = bitcast i8* [[TMP0]] to i32*
 // CHECK:   [[TMP2:%.*]] = load i32, i32* [[TMP1]], align 4
-// CHECK:   [[TMP3:%.*]] = insertelement <4 x i32> undef, i32 [[TMP2]], i32 0
+// CHECK:   [[TMP3:%.*]] = insertelement <4 x i32> poison, i32 [[TMP2]], i32 0
 // CHECK:   [[LANE:%.*]] = shufflevector <4 x i32> [[TMP3]], <4 x i32> [[TMP3]], <4 x i32> zeroinitializer
 // CHECK:   ret <4 x i32> [[LANE]]
 int32x4_t test_vld1q_dup_s32(int32_t const * a) {
@@ -4152,7 +4152,7 @@
 // CHECK:   [[TMP0:%.*]] = bitcast i64* %a to i8*
 // CHECK:   [[TMP1:%.*]] = bitcast i8* [[TMP0]] to i64*
 // CHECK:   [[TMP2:%.*]] = load i64, i64* [[TMP1]], align 4
-// CHECK:   [[TMP3:%.*]] = insertelement <2 x 

[PATCH] D135392: Use PoisonValue in vector BIs [NFC]

[Nuno Lopes via Phabricator via cfe-commits]

nlopes accepted this revision.
nlopes 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/D135392/new/

https://reviews.llvm.org/D135392

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

[PATCH] D135091: Create storage for the `_cmd` argument to the helper function for generated getters/setters of `direct` Objective-C properties.

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added inline comments.



Comment at: clang/lib/CodeGen/CGObjC.cpp:1125
+llvm::Type *selType = CGF.ConvertType(CGF.getContext().getObjCSelType());
+return llvm::UndefValue::get(selType);
+  }

Please consider using PoisonValue here instead (if appropriate). We are trying 
to get rid of undef.
Thank you!


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

https://reviews.llvm.org/D135091

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

[PATCH] D90275: [clang][IR] Add support for leaf attribute

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D90275#3712969 , @gulfem wrote:

> In D90275#3712880 , @nlopes wrote:
>
>> In D90275#3707793 , @nlopes wrote:
>>
>>> In D90275#3671983 , @gulfem wrote:
>>>
 In D90275#3671019 , @nlopes wrote:

> Could you please add a description of this attribute to LangRef?
> We need the semantics of this.
>
> Thank you!

 Sure, I'll do that!
>>>
>>> ping?
>>
>> Since the patch author has been ignoring my ask (and is active), and since 
>> documenting IR features is of paramount importance, I suggest we revert this 
>> patch. I think 3 weeks to document something is sufficient, and the author 
>> didn't provide an ETA.
>> I would like to ask everyone to reject all future patches that change the IR 
>> and don't include documentation, as experience says that we'll get 
>> miscompilations due to different people having different ideas of the 
>> semantics.
>>
>> TL;DR: I'll revert this patch on Friday if no one chimes in. Thank you!
>
> I'm very sorry and I was not ignoring your ask. I'll do it now.

Thanks!
I'm fine with a reasonable ETA, but we really need all IR features documented 
to avoid bugs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90275

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

[PATCH] D90275: [clang][IR] Add support for leaf attribute

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D90275#3707793 , @nlopes wrote:

> In D90275#3671983 , @gulfem wrote:
>
>> In D90275#3671019 , @nlopes wrote:
>>
>>> Could you please add a description of this attribute to LangRef?
>>> We need the semantics of this.
>>>
>>> Thank you!
>>
>> Sure, I'll do that!
>
> ping?

Since the patch author has been ignoring my ask (and is active), and since 
documenting IR features is of paramount importance, I suggest we revert this 
patch. I think 3 weeks to document something is sufficient, and the author 
didn't provide an ETA.
I would like to ask everyone to reject all future patches that change the IR 
and don't include documentation, as experience says that we'll get 
miscompilations due to different people having different ideas of the semantics.

TL;DR: I'll revert this patch on Friday if no one chimes in. Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90275

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

[PATCH] D131547: [Clang][AArch64] Use generic extract/insert vector for svget/svset/svcreate tuples

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added inline comments.



Comment at: clang/lib/CodeGen/CGBuiltin.cpp:9114
+  unsigned int MinElts = SrcTy->getElementCount().getKnownMinValue();
+  Value *Call = llvm::UndefValue::get(VTy);
+  for (unsigned int I = 0; I < Ops.size(); I++) {

Please use PoisonValue here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131547

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

[PATCH] D90275: [clang][IR] Add support for leaf attribute

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D90275#3671983 , @gulfem wrote:

> In D90275#3671019 , @nlopes wrote:
>
>> Could you please add a description of this attribute to LangRef?
>> We need the semantics of this.
>>
>> Thank you!
>
> Sure, I'll do that!

ping?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90275

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

[PATCH] D126745: [RISCV][Clang] Support policy functions for vmerge, vfmerge and vcompress.

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D126745#3693790 , @khchen wrote:

> @nlopes we will update all undef to poison in follow up patches.

Thank you!
The benefit is that we want to remove undef altogether, so I'm trying to avoid 
having more undefs introduced, and if possible have everyone help reducing its 
usage whenever possible.
Thank you for your help!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126745

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

[PATCH] D130224: [Clang][Attribute] Introduce maybe_undef attribute for function arguments which accepts undef values

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D130224#3687351 , @arsenm wrote:

> In D130224#3686756 , @nlopes wrote:
>
>> Would it be possible to implement this by dropping the `noundef` attribute 
>> rather than emitting a freeze? Seems like a much better option to me.
>
> See @nhaehnle's post here 
> 

Thank you for the reference!
Seems like a big hammer, but I don't know the semantics of the GPU languages to 
comment further.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130224

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

[PATCH] D130224: [Clang][Attribute] Introduce maybe_undef attribute for function arguments which accepts undef values

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

Would it be possible to implement this by dropping the `noundef` attribute 
rather than emitting a freeze? Seems like a much better option to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130224

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

[PATCH] D126745: [RISCV][Clang] Support policy functions for vmerge, vfmerge and vcompress.

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

While at it, could you switch those UndefValue with PoisonValue if possible?  
Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126745

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

[PATCH] D126748: [RISCV][Clang] Support policy functions for Vector Reduction Instructions.

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

While at it, could you switch those UndefValue with PoisonValue if possible?  
Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126748

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

[PATCH] D126750: [RISCV][Clang] Support policy function for all vector segment load.

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

While at it, could you switch those UndefValue with PoisonValue if possible?  
Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126750

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

[PATCH] D90275: [clang][IR] Add support for leaf attribute

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.
Herald added a subscriber: ormris.
Herald added a project: All.

Could you please add a description of this attribute to LangRef?
We need the semantics of this.

Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90275

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

[PATCH] D128501: [CodeGen] Make uninitialized Lvalue bit-field stores poison compatible

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D128501#3613420 , @efriedma wrote:

>> No, you can still link those. There's no ABI change nor any interference at 
>> IR level.
>
> The scenario I was thinking of with -ffine-grained-bitfield-accesses is 
> something like the following:
>
> File A:
>
>   struct X { int a : 8; int b : 24; };
>   void f(struct X*);
>   int g() {
>   struct X x;
>   x.a = 10;
>   f();
>   return x.a;
>   }
>
> File B:
>
>   struct X { int a : 8; int b : 24; };
>   void f(struct X* x) {
>   x->b = 10;
>   }
>
> If both files are compiled -ffine-grained-bitfield-accesses, the fields don't 
> overlap.  If both files are compiled with 
> -fno-fine-grained-bitfield-accesses, the assignment in file A freezes both 
> fields of "x".  If file A is compiled with -ffine-grained-bitfield-accesses, 
> but file B is not, f() corrupts the field "a", so g() returns poison (if I'm 
> not missing anything?).

You are right, thanks! f() corrupts `a` because f assumes the fields were both 
initialized or neither of them was initialized.
The fix is not trivial though, because -ffine-grained-bitfield-accesses would 
have to freeze the adjacent fields.
This only matters if the IRs are linked together with IPO. Otherwise, at object 
level it doesn't really matter.

Do you think we can get away by just documenting the incompatibility of doing 
IPO with files compiled with different -ffine-grained-bitfield-accesses flags?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128501

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

[PATCH] D128501: [CodeGen] Make uninitialized Lvalue bit-field stores poison compatible

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D128501#3608846 , @efriedma wrote:

> Under this scheme, is it illegal to link together object files built with 
> -ffine-grained-bitfield-accesses and object files built with 
> -fno-fine-grained-bitfield-accesses?

No, you can still link those. There's no ABI change nor any interference at IR 
level.
Thanks for the pointer, I wasn't aware of that option. What we can do is to 
optimize away the freezes with -ffine-grained-bitfield-accesses. The freeze is 
only needed when a struct field is shared between more than one bitfield.

Regarding perf, it's looking good, but @jmciver will post here a summary later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128501

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

[PATCH] D125983: [DeadArgElim] Use poison instead of undef as placeholder for dead arguments

[Nuno Lopes 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 rG5fc9449c962a: [DeadArgElim] Use poison instead of undef as 
placeholder for dead arguments (authored by nlopes).
Herald added subscribers: cfe-commits, atanasyan, jrtc27.
Herald added a project: clang.

Changed prior to commit:
  https://reviews.llvm.org/D125983?vs=430702=430729#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125983

Files:
  clang/test/CodeGen/debug-info-block-vars.c
  clang/test/CodeGen/mips-unsigned-ext-var.c
  llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
  llvm/test/DebugInfo/X86/dbgloc-insert-extract-val-instrs.ll
  llvm/test/Transforms/DeadArgElim/NoundefAttrs.ll
  llvm/test/Transforms/DeadArgElim/aggregates.ll
  llvm/test/Transforms/DeadArgElim/byref.ll
  llvm/test/Transforms/DeadArgElim/dbginfo-update-dbgval-local.ll
  llvm/test/Transforms/DeadArgElim/dbginfo-update-dbgval.ll
  llvm/test/Transforms/DeadArgElim/deadexternal.ll
  llvm/test/Transforms/DeadArgElim/fct_ptr.ll
  llvm/test/Transforms/DeadArgElim/opaque-ptr.ll
  llvm/test/Transforms/DeadArgElim/variadic_safety.ll

Index: llvm/test/Transforms/DeadArgElim/variadic_safety.ll
===
--- llvm/test/Transforms/DeadArgElim/variadic_safety.ll
+++ llvm/test/Transforms/DeadArgElim/variadic_safety.ll
@@ -17,9 +17,9 @@
 define i32 @call_va(i32 %in) {
   %stacked = alloca i32
   store i32 42, i32* %stacked
-  %res = call i32(i32, i32, ...) @va_func(i32 %in, i32 %in, [6 x i32] undef, i32* byval(i32) %stacked)
+  %res = call i32(i32, i32, ...) @va_func(i32 %in, i32 %in, [6 x i32] poison, i32* byval(i32) %stacked)
   ret i32 %res
-; CHECK: call i32 (i32, i32, ...) @va_func(i32 undef, i32 %in, [6 x i32] undef, i32* byval(i32) %stacked)
+; CHECK: call i32 (i32, i32, ...) @va_func(i32 poison, i32 %in, [6 x i32] poison, i32* byval(i32) %stacked)
 }
 
 define internal i32 @va_deadret_func(i32 %a, i32 %b, ...) {
@@ -32,7 +32,7 @@
 define void @call_deadret(i32 %in) {
   %stacked = alloca i32
   store i32 42, i32* %stacked
-  call i32 (i32, i32, ...) @va_deadret_func(i32 undef, i32 %in, [6 x i32] undef, i32* byval(i32) %stacked)
+  call i32 (i32, i32, ...) @va_deadret_func(i32 poison, i32 %in, [6 x i32] poison, i32* byval(i32) %stacked)
   ret void
-; CHECK: call void (i32, i32, ...) @va_deadret_func(i32 undef, i32 undef, [6 x i32] undef, i32* byval(i32) %stacked)
+; CHECK: call void (i32, i32, ...) @va_deadret_func(i32 poison, i32 poison, [6 x i32] poison, i32* byval(i32) %stacked)
 }
Index: llvm/test/Transforms/DeadArgElim/opaque-ptr.ll
===
--- llvm/test/Transforms/DeadArgElim/opaque-ptr.ll
+++ llvm/test/Transforms/DeadArgElim/opaque-ptr.ll
@@ -11,7 +11,7 @@
 
 define void @caller() {
 ; CHECK-LABEL: define {{[^@]+}}@caller() {
-; CHECK-NEXT:call void @callee(i32 undef)
+; CHECK-NEXT:call void @callee(i32 poison)
 ; CHECK-NEXT:call void @callee()
 ; CHECK-NEXT:call void @callee(i32 42, i32 24)
 ; CHECK-NEXT:ret void
Index: llvm/test/Transforms/DeadArgElim/fct_ptr.ll
===
--- llvm/test/Transforms/DeadArgElim/fct_ptr.ll
+++ llvm/test/Transforms/DeadArgElim/fct_ptr.ll
@@ -5,7 +5,7 @@
 ; Because of that use, we used to bail out on removing the
 ; unused arguments for this function.
 ; Yet, we should still be able to rewrite the direct calls that are
-; statically known, by replacing the related arguments with undef.
+; statically known, by replacing the related arguments with poison.
 ; This is what we check on the call that produces %res2.
 
 define i32 @call_indirect(i32 (i32, i32, i32)* readnone %fct_ptr, i32 %arg1, i32 %arg2, i32 %arg3) {
@@ -13,13 +13,13 @@
 ; CHECK-NEXT:[[CMP0:%.*]] = icmp eq i32 (i32, i32, i32)* [[FCT_PTR:%.*]], @external_fct
 ; CHECK-NEXT:br i1 [[CMP0]], label [[CALL_EXT:%.*]], label [[CHK2:%.*]]
 ; CHECK:   call_ext:
-; CHECK-NEXT:[[RES1:%.*]] = tail call i32 @external_fct(i32 undef, i32 [[ARG2:%.*]], i32 undef)
+; CHECK-NEXT:[[RES1:%.*]] = tail call i32 @external_fct(i32 poison, i32 [[ARG2:%.*]], i32 poison)
 ; CHECK-NEXT:br label [[END:%.*]]
 ; CHECK:   chk2:
 ; CHECK-NEXT:[[CMP1:%.*]] = icmp eq i32 (i32, i32, i32)* [[FCT_PTR]], @internal_fct
 ; CHECK-NEXT:br i1 [[CMP1]], label [[CALL_INT:%.*]], label [[CALL_OTHER:%.*]]
 ; CHECK:   call_int:
-; CHECK-NEXT:[[RES2:%.*]] = tail call i32 @internal_fct(i32 undef, i32 [[ARG2]], i32 undef)
+; CHECK-NEXT:[[RES2:%.*]] = tail call i32 @internal_fct(i32 poison, i32 [[ARG2]], i32 poison)
 ; CHECK-NEXT:br label [[END]]
 ; CHECK:   call_other:
 ; CHECK-NEXT:[[RES3:%.*]] = tail call i32 @other_fct(i32 [[ARG2]])
Index: llvm/test/Transforms/DeadArgElim/deadexternal.ll

[PATCH] D98152: [InstCombine] Canonicalize SPF to min/max intrinsics

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

This fixed 9 bugs reports by Alive2:

- Transforms/InstCombine/max-of-nots.ll
- Transforms/InstCombine/max_known_bits.ll
- Transforms/InstCombine/minmax-fold.ll
- Transforms/InstCombine/saturating-add-sub.ll
- Transforms/InstCombine/select-pr39595.ll
- Transforms/InstCombine/select_meta.ll
- Transforms/InstCombine/sext.ll
- Transforms/InstCombine/sub.ll
- Transforms/InstCombine/with_overflow.ll

Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98152

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

[PATCH] D119816: Fix not working attribute no_sanitize bounds that affects linux kernel

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D119816#3332414 , @ztong0001 wrote:

> In D119816#3331658 , @nlopes wrote:
>
>> Well, this patch is just a band-aid and a disaster waiting to happen.
>> If kmalloc is tagged with an `__attribute__` stating the allocation size, 
>> then you can't dereference beyond that limit or risk the alias analysis do 
>> something you don't want. You are triggering UB like that.
>> Can't you just remove the `__attribute__` from kmalloc instead to avoid 
>> triggering UB?
>
> I am not sure I fully get what you are saying.
> What I am suggesting is something like below to avoid bounds check pass from 
> running on this function.
>
>   diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>   index 9d0388bed0c1..186fca35266d 100644
>   --- a/net/core/skbuff.c
>   +++ b/net/core/skbuff.c
>   @@ -1679,7 +1679,7 @@ EXPORT_SYMBOL(__pskb_copy_fclone);
> * All the pointers pointing into skb header may change and must be
> * reloaded after call to this function.
> */
>   -
>   +__attribute__((no_sanitize("bounds")))
>int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
>gfp_t gfp_mask)
>{

The main issue is that the kernel is wrong. It has a bug. The sanitizer's error 
is not a false-positive!
So what you are proposing is a band-aid. It's not a real solution and it's just 
masking a wider problem. LLVM knows that kmalloc(x) allocates x bytes because 
someone placed an `__attribute__ ((alloc_size (1)))` on kmalloc. That attribute 
is just wrong and should be removed. It allows LLVM to mark all accesses beyond 
`kmalloc(x) + x - 1` as undefined behavior.

TL;DR: this patch is not the solution for your problems.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119816

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

[PATCH] D119816: Fix not working attribute no_sanitize bounds that affects linux kernel

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

Well, this patch is just a band-aid and a disaster waiting to happen.
If kmalloc is tagged with an __attribute__ stating the allocation size, then 
you can't dereference beyond that limit or risk the alias analysis do something 
you don't want. You are triggering UB like that.
Can't you just remove the __attribute__ from kmalloc instead to avoid 
triggering UB?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119816

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

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2535
 ArgAttrs[FirstIRArg + i] =
 llvm::AttributeSet::get(getLLVMContext(), Attrs);
 }

sammccall wrote:
> fhahn wrote:
> > nlopes wrote:
> > > fhahn wrote:
> > > > nlopes wrote:
> > > > > ab wrote:
> > > > > > Hmm, if I'm reading this right, this overwrites the `nonnull 
> > > > > > dereferenceable align` attributes separately computed for `this` 
> > > > > > around l2335, right? (or `inalloca` and `sret` before that)
> > > > > > 
> > > > > > It sounds like an ancient bug, that's only exposed because 
> > > > > > `noundef` ends up triggering this logic much more often?
> > > > > > 
> > > > > > Many of our downstream tests hit this. The hacked up patch seems to 
> > > > > > do the job; ideally we'd feed the previously-computed attrs when 
> > > > > > constructing the AttrBuilder (which would also fix the padding 
> > > > > > argument), but we'd need to match up the IR args first.  Maybe 
> > > > > > that's fine as a special-case for arg 0 though
> > > > > nice catch! It's an ancient bug where arg 0 is overwritten.
> > > > Is anybody looking into a fix or should we revert the patch until this 
> > > > can be properly addressed?
> > > I would recommend against reverting this patch because of all the 
> > > followup patches. There were quite a few commits afterwards plus fixes to 
> > > buildbot configurations, so it's a non-trivial overhead to revert 
> > > everything.
> > > I was assuming @ab would fix it as he already root-caused the bug..
> > FWIW it seems a bit unfortunate that there are some clear regressions when 
> > the tests got update, e.g. 
> > https://github.com/llvm/llvm-project/commit/1b1c8d83d3567a60280291c0adb95d1d60335509#diff-7e80d593f26f6f6fb24765c6a169884d7350685d565ee970b0a7b9abaf0fb205L355
> > 
> > I'll work with @ab to fix this though rather than reverting, because 
> > another revert would cause even more conflicts for us downstream.
> > FWIW it seems a bit unfortunate that there are some clear regressions when 
> > the tests got update, e.g. 
> > https://github.com/llvm/llvm-project/commit/1b1c8d83d3567a60280291c0adb95d1d60335509#diff-7e80d593f26f6f6fb24765c6a169884d7350685d565ee970b0a7b9abaf0fb205L355
> > 
> > I'll work with @ab to fix this though rather than reverting, because 
> > another revert would cause even more conflicts for us downstream.
> 
> Just a reminder that the 14 release is cut soon (1 feb: 
> https://llvm.discourse.group/t/llvm-14-0-0-release-schedule/5846)
> 
> I don't know this code; if a clean fix is ready soon and unlikely to have a 
> ripple effect then great. But it does seem risky to be treating such 
> configuration changes as "too big to fail" at this point in the release cycle.
Thank you Florian!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105169

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

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D105169#3257849 , @sdesmalen wrote:

> In D105169#3254757 , @nlopes wrote:
>
>> In D105169#3254692 , 
>> @DavidSpickett wrote:
>>
>>> Hi, this patch has caused a gcc test suite failure on our SVE bots: 
>>> https://lab.llvm.org/buildbot/#/builders/184/builds/1941
>>> (test source is 
>>> https://github.com/llvm/llvm-test-suite/blob/main/SingleSource/Regression/C/gcc-c-torture/execute/pr79286.c)
>>>
>>> I've only just been made aware of the failure so we will spend some time to 
>>> find out the reason before asking that anything be changed. Could be 
>>> something specific to SVE in which case we'll handle it.
>>
>> Thanks!
>> That test has UB in the printf class as it reads a non-initialized variable. 
>> Can you check if changing line 6 to 'int e = 0;' solves the problem?
>
> I can confirm that fixes the issue.

Thank you! I've committed the fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105169

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

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2535
 ArgAttrs[FirstIRArg + i] =
 llvm::AttributeSet::get(getLLVMContext(), Attrs);
 }

fhahn wrote:
> nlopes wrote:
> > ab wrote:
> > > Hmm, if I'm reading this right, this overwrites the `nonnull 
> > > dereferenceable align` attributes separately computed for `this` around 
> > > l2335, right? (or `inalloca` and `sret` before that)
> > > 
> > > It sounds like an ancient bug, that's only exposed because `noundef` ends 
> > > up triggering this logic much more often?
> > > 
> > > Many of our downstream tests hit this. The hacked up patch seems to do 
> > > the job; ideally we'd feed the previously-computed attrs when 
> > > constructing the AttrBuilder (which would also fix the padding argument), 
> > > but we'd need to match up the IR args first.  Maybe that's fine as a 
> > > special-case for arg 0 though
> > nice catch! It's an ancient bug where arg 0 is overwritten.
> Is anybody looking into a fix or should we revert the patch until this can be 
> properly addressed?
I would recommend against reverting this patch because of all the followup 
patches. There were quite a few commits afterwards plus fixes to buildbot 
configurations, so it's a non-trivial overhead to revert everything.
I was assuming @ab would fix it as he already root-caused the bug..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105169

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

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D105169#3254692 , @DavidSpickett 
wrote:

> Hi, this patch has caused a gcc test suite failure on our SVE bots: 
> https://lab.llvm.org/buildbot/#/builders/184/builds/1941
> (test source is 
> https://github.com/llvm/llvm-test-suite/blob/main/SingleSource/Regression/C/gcc-c-torture/execute/pr79286.c)
>
> I've only just been made aware of the failure so we will spend some time to 
> find out the reason before asking that anything be changed. Could be 
> something specific to SVE in which case we'll handle it.

Thanks!
That test has UB in the printf class as it reads a non-initialized variable. 
Can you check if changing line 6 to 'int e = 0;' solves the problem?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105169

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

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added inline comments.



Comment at: clang/lib/CodeGen/CGCall.cpp:2535
 ArgAttrs[FirstIRArg + i] =
 llvm::AttributeSet::get(getLLVMContext(), Attrs);
 }

ab wrote:
> Hmm, if I'm reading this right, this overwrites the `nonnull dereferenceable 
> align` attributes separately computed for `this` around l2335, right? (or 
> `inalloca` and `sret` before that)
> 
> It sounds like an ancient bug, that's only exposed because `noundef` ends up 
> triggering this logic much more often?
> 
> Many of our downstream tests hit this. The hacked up patch seems to do the 
> job; ideally we'd feed the previously-computed attrs when constructing the 
> AttrBuilder (which would also fix the padding argument), but we'd need to 
> match up the IR args first.  Maybe that's fine as a special-case for arg 0 
> though
nice catch! It's an ancient bug where arg 0 is overwritten.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105169

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

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D105169#3244624 , @nathanchance 
wrote:

> I can reduce all of these down for you and/or I can start an email thread 
> with the `objtool` maintainers to see if there is a way to fix or avoid these 
> warnings on the `objtool` side and include you in that discussion, if LLVM is 
> not really doing anything wrong here. I am by no means an expert in this area 
> and I don't want to delay this anymore but I want to avoid regressing our 
> builds, as `objtool` regularly helps us spot compiler bugs. Perhaps 
> @nickdesaulniers has some other thoughts?

LLVM is not doing anything wrong here. The issue is that once we have UB, LLVM 
is not required to lay out the functions nicely. For example, issue #53118 is 
just a "nice to fix", it's not a bug.
On the other hand, I understand that fixing `objtool` is likely impossible if 
we consider this UB thing as assembly doesn't have enough information to 
distinguish between a compiler bug and a UB case. I just don't know how 
frequent and/or useful these warnings are.

I don't think we should hold this patch anymore as it's not wrong and the 
side-effects are understood. We can and should try to reduce those kernel 
warnings to zero, but we cannot put all that burden on this patch's author.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105169

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

[PATCH] D116887: [SROA] Switch replacement of dead/UB/unreachable ops from undef to poison

[Nuno Lopes 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 rG7b1cb72ad944: [SROA] Switch replacement of 
dead/UB/unreachable ops from undef to poison (authored by nlopes).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D116887?vs=398416=398605#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116887

Files:
  clang/test/CodeGen/aarch64-bf16-ldst-intrinsics.c
  llvm/lib/Transforms/Scalar/SROA.cpp
  llvm/test/Transforms/SROA/basictest-opaque-ptrs.ll
  llvm/test/Transforms/SROA/basictest.ll
  llvm/test/Transforms/SROA/non-capturing-call.ll
  llvm/test/Transforms/SROA/phi-and-select.ll

Index: llvm/test/Transforms/SROA/phi-and-select.ll
===
--- llvm/test/Transforms/SROA/phi-and-select.ll
+++ llvm/test/Transforms/SROA/phi-and-select.ll
@@ -237,8 +237,8 @@
 define i32 @test6(i32* %b) {
 ; CHECK-LABEL: @test6(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:[[SELECT2:%.*]] = select i1 false, i32* undef, i32* [[B:%.*]]
-; CHECK-NEXT:[[SELECT3:%.*]] = select i1 false, i32* undef, i32* [[B]]
+; CHECK-NEXT:[[SELECT2:%.*]] = select i1 false, i32* poison, i32* [[B:%.*]]
+; CHECK-NEXT:[[SELECT3:%.*]] = select i1 false, i32* poison, i32* [[B]]
 ; CHECK-NEXT:call void @f(i32* [[SELECT2]], i32* [[SELECT3]])
 ; CHECK-NEXT:ret i32 1
 ;
@@ -272,7 +272,7 @@
 ; CHECK:   good:
 ; CHECK-NEXT:br label [[EXIT:%.*]]
 ; CHECK:   bad:
-; CHECK-NEXT:[[P_SROA_SPECULATE_LOAD_BAD:%.*]] = load i32, i32* undef, align 4
+; CHECK-NEXT:[[P_SROA_SPECULATE_LOAD_BAD:%.*]] = load i32, i32* poison, align 4
 ; CHECK-NEXT:br label [[EXIT]]
 ; CHECK:   exit:
 ; CHECK-NEXT:[[P_SROA_SPECULATED:%.*]] = phi i32 [ 0, [[GOOD]] ], [ [[P_SROA_SPECULATE_LOAD_BAD]], [[BAD]] ]
@@ -525,7 +525,7 @@
 ; CHECK:   loop2:
 ; CHECK-NEXT:br i1 undef, label [[LOOP1]], label [[EXIT]]
 ; CHECK:   exit:
-; CHECK-NEXT:[[PHI2:%.*]] = phi i32* [ undef, [[LOOP2]] ], [ null, [[ENTRY:%.*]] ]
+; CHECK-NEXT:[[PHI2:%.*]] = phi i32* [ poison, [[LOOP2]] ], [ null, [[ENTRY:%.*]] ]
 ; CHECK-NEXT:ret i32 undef
 ;
 
Index: llvm/test/Transforms/SROA/non-capturing-call.ll
===
--- llvm/test/Transforms/SROA/non-capturing-call.ll
+++ llvm/test/Transforms/SROA/non-capturing-call.ll
@@ -450,7 +450,7 @@
 ; CHECK-NEXT:[[I0:%.*]] = call i32 @user_of_alloca(i32* nocapture nonnull [[RETVAL]])
 ; CHECK-NEXT:[[I1_FCA_0_GEP:%.*]] = getelementptr inbounds [2 x i32], [2 x i32]* [[RETVAL_FULL]], i32 0, i32 0
 ; CHECK-NEXT:[[I1_FCA_0_LOAD:%.*]] = load i32, i32* [[I1_FCA_0_GEP]], align 4
-; CHECK-NEXT:[[I1_FCA_0_INSERT:%.*]] = insertvalue [2 x i32] undef, i32 [[I1_FCA_0_LOAD]], 0
+; CHECK-NEXT:[[I1_FCA_0_INSERT:%.*]] = insertvalue [2 x i32] poison, i32 [[I1_FCA_0_LOAD]], 0
 ; CHECK-NEXT:[[I1_FCA_1_GEP:%.*]] = getelementptr inbounds [2 x i32], [2 x i32]* [[RETVAL_FULL]], i32 0, i32 1
 ; CHECK-NEXT:[[I1_FCA_1_LOAD:%.*]] = load i32, i32* [[I1_FCA_1_GEP]], align 4
 ; CHECK-NEXT:[[I1_FCA_1_INSERT:%.*]] = insertvalue [2 x i32] [[I1_FCA_0_INSERT]], i32 [[I1_FCA_1_LOAD]], 1
@@ -479,7 +479,7 @@
 ; CHECK-OPAQUE-NEXT:[[I0:%.*]] = call i32 @user_of_alloca(ptr nocapture nonnull [[RETVAL]])
 ; CHECK-OPAQUE-NEXT:[[I1_FCA_0_GEP:%.*]] = getelementptr inbounds [2 x i32], ptr [[RETVAL_FULL]], i32 0, i32 0
 ; CHECK-OPAQUE-NEXT:[[I1_FCA_0_LOAD:%.*]] = load i32, ptr [[I1_FCA_0_GEP]], align 4
-; CHECK-OPAQUE-NEXT:[[I1_FCA_0_INSERT:%.*]] = insertvalue [2 x i32] undef, i32 [[I1_FCA_0_LOAD]], 0
+; CHECK-OPAQUE-NEXT:[[I1_FCA_0_INSERT:%.*]] = insertvalue [2 x i32] poison, i32 [[I1_FCA_0_LOAD]], 0
 ; CHECK-OPAQUE-NEXT:[[I1_FCA_1_GEP:%.*]] = getelementptr inbounds [2 x i32], ptr [[RETVAL_FULL]], i32 0, i32 1
 ; CHECK-OPAQUE-NEXT:[[I1_FCA_1_LOAD:%.*]] = load i32, ptr [[I1_FCA_1_GEP]], align 4
 ; CHECK-OPAQUE-NEXT:[[I1_FCA_1_INSERT:%.*]] = insertvalue [2 x i32] [[I1_FCA_0_INSERT]], i32 [[I1_FCA_1_LOAD]], 1
@@ -533,7 +533,7 @@
 ; CHECK-NEXT:[[I0:%.*]] = call i32 @user_of_alloca_with_multiple_args(i32* nocapture nonnull [[RETVAL]], i32* nocapture nonnull [[RETVAL_BASE]])
 ; CHECK-NEXT:[[I1_FCA_0_GEP:%.*]] = getelementptr inbounds [2 x i32], [2 x i32]* [[RETVAL_FULL]], i32 0, i32 0
 ; CHECK-NEXT:[[I1_FCA_0_LOAD:%.*]] = load i32, i32* [[I1_FCA_0_GEP]], align 4
-; CHECK-NEXT:[[I1_FCA_0_INSERT:%.*]] = insertvalue [2 x i32] undef, i32 [[I1_FCA_0_LOAD]], 0
+; CHECK-NEXT:[[I1_FCA_0_INSERT:%.*]] = insertvalue [2 x i32] poison, i32 [[I1_FCA_0_LOAD]], 0
 ; CHECK-NEXT:[[I1_FCA_1_GEP:%.*]] = getelementptr inbounds [2 x i32], [2 x i32]* [[RETVAL_FULL]], i32 0, i32 1
 ; CHECK-NEXT:[[I1_FCA_1_LOAD:%.*]] = load i32, i32* 

[PATCH] D110745: Redefine deref(N) attribute family as point-in-time semantics (aka deref-at-point)

[Nuno Lopes via Phabricator via cfe-commits]

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

LGTM. We have discussed this before. It's important to fix the reference types 
in C++.




Comment at: llvm/test/Analysis/BasicAA/dereferenceable.ll:66
 ; CHECK: Function: local_and_deref_ret_2: 2 pointers, 2 call sites
-; CHECK-NEXT:  NoAlias:i32* %obj, i32* %ret
+; CHECK-NEXT:  MayAlias:   i32* %obj, i32* %ret
 bb:

this one is unfortunate.. They can't alias because objsize(%obj) < 
dereferenceable size of %ret. This case should be trivial to catch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110745

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

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

(repost my message from llvm-dev)

I can add one thought regarding why this direction makes sense and why it 
doesn't constrain optimizations.

Traditionally we don't want to mark too many things as UB as it restricts code 
movement and thus limits optimizations. That's why we have poison for e.g. 
signed arithmetic overflow rather than using UB as allowed by the C standard.
For function calls, however, optimizers are already super constrained: in 
general we cannot move them around because they may not return, they may throw 
an exception, they may modify memory, do a syscall, and so on. So adding 
another restriction to function calls isn't a big deal; optimizers don't touch 
them that much anyway.

This proposal adds more UB, as it turns undef/poison into UB on function calls, 
but that doesn't limit optimizations. So it seems like a good tradeoff: we 
learn some values can't be undef/poison "for free".  Plus that UB can be 
dropped if needed; dropping noundef is legal! So there are really no downsides.
That's why I believe this is a good direction for clang.

From the users perspective, hopefully the sanitizers can already flag related 
issues so hopefully this won't lead to hard-to-debug UB bugs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105169

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

[PATCH] D99121: [IR][InstCombine] IntToPtr Produces Typeless Pointer To Byte

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

Given that it gives a decent speedup for some important workload, and provided 
it doesn't regress others, I think this should go in then.
It's easy to revert this once opaque pointers arrive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99121

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

[PATCH] D99121: [IR][InstCombine] IntToPtr Produces Typeless Pointer To Byte

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D99121#2645593 , @dblaikie wrote:

> In D99121#2644362 , @lebedev.ri 
> wrote:
>
>> In D99121#2644223 , @nlopes wrote:
>>
>>> The pointee type in LLVM doesn't really matter. It's even supposed to 
>>> disappear one day after the migration is completed.
>>> E.g., i8* and i64* are exactly the same thing: they are pointers to data.
>>
>> Yep. That will be indeed a great to see.
>>
>>> So, I don't understand the motivation for this patch. It doesn't solve the 
>>> root cause of the problem (which one btw?).
>>
>> It is indeed temporary until Opaque pointers are here.
>> The problem has been stated last time in D99051 
>>  by @ruiling:
>> https://godbolt.org/z/x7E1EjWvv, i.e. given the same integer,
>> there can be any number of pointers `inttoptr`'d from it,
>> and passes won't be able to tell that they are identical.
>>
>> @dblaikie @t.p.northover can anyone comment on the Opaque Pointers progress? 
>> Is there a checklist somewhere?
>
> no checklist, unfortunately - myself, @t.p.northover, @jyknight, and @arsenm 
> have all done bits and pieces of work on it lately.
>
> I think we've got most of the big IR changes (adding explicit types where 
> they'll be needed when they're no longer carried on the type of pointer 
> parameters) - @arsenm's D98146  is another 
> piece in that area, hopefully near the last I think.
>
> After all that's in place, the next step I think would be to introduce the 
> typeless pointer, support it as an operand to these various operations - and 
> then try producing it as a result of instructions too. But I'm probably 
> missing a bunch of important steps we'll find are necessary...

Do you have an ETA for when the switch will happen? Just to inform us where we 
should proceed with temp fixes like this one or we should just wait.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99121

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

[PATCH] D99121: [IR][InstCombine] IntToPtr Produces Typeless Pointer To Byte

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

The pointee type in LLVM doesn't really matter. It's even supposed to disappear 
one day after the migration is completed.
E.g., i8* and i64* are exactly the same thing: they are pointers to data.
So, I don't understand the motivation for this patch. It doesn't solve the root 
cause of the problem (which one btw?).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99121

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

[PATCH] D81678: Introduce noundef attribute at call sites for stricter poison analysis

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

@rsmith already gave his blessing, so please go ahead.

I hope you guys have a plan to enable this by default at some point as features 
behind a flag get rotten and no one uses them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81678

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

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D87188#2476230 , @thakis wrote:

> In D87188#2470401 , @lebedev.ri 
> wrote:
>
>> In D87188#2470392 , @thakis wrote:
>>
>>> Heads up: Breaks a test for us: 
>>> https://bugs.chromium.org/p/chromium/issues/detail?id=1161542
>>>
>>> (No reduced repro yet, might be UB, just fyi at this point.)
>>
>> Thanks for headsup. For now i'll deal with the problem @nlopes pointed out 
>> above in a bit..
>
> Just to follow up, this ended up being UB on our end (fix: 
> https://android-review.googlesource.com/c/platform/external/perfetto/+/1535483)

Nice!   Thanks for the update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87188

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

[PATCH] D93793: [IR] Let IRBuilder's CreateVectorSplat use poison as inselt's placeholder

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added inline comments.



Comment at: llvm/lib/IR/IRBuilder.cpp:1021
   Value *Zeros = ConstantAggregateZero::get(VectorType::get(I32Ty, EC));
   return CreateShuffleVector(V, Undef, Zeros, Name + ".splat");
 }

while at it, don't you want to change this one to poison as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93793

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

[PATCH] D87188: [InstCombine] Canonicalize SPF to abs intrinc

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

This patch regressed the following test of Transforms/InstCombine/abs-1.ll:
(need to drop NSW in this case).

  define i8 @nabs_canonical_3(i8 %x) {
  %0:
%cmp = icmp slt i8 %x, 0
%neg = sub nsw i8 0, %x
%abs = select i1 %cmp, i8 %x, i8 %neg
ret i8 %abs
  }
  =>
  define i8 @nabs_canonical_3(i8 %x) {
  %0:
%1 = abs i8 %x, 1
%abs = sub nsw i8 0, %1
ret i8 %abs
  }
  Transformation doesn't verify!
  ERROR: Target is more poisonous than source
  
  Example:
  i8 %x = #x80 (128, -128)
  
  Source:
  i1 %cmp = #x1 (1)
  i8 %neg = poison
  i8 %abs = #x80 (128, -128)
  
  Target:
  i8 %1 = poison
  i8 %abs = poison
  Source value: #x80 (128, -128)
  Target value: poison

(same bug occurs with @nabs_nabs_x01 in Transforms/InstCombine/abs_abs.ll)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87188

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

[PATCH] D78938: Make LLVM build in C++20 mode

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D78938#2459973 , @RKSimon wrote:

> @BRevzin @nlopes This is causing MSVC build failure please can you take a 
> look?
>
>   E:\llvm\llvm-project\llvm\include\llvm/DebugInfo/DWARF/DWARFDie.h(405): 
> note: see declaration of 'std::reverse_iterator'
>   E:\llvm\llvm-project\llvm\lib\DWARFLinker\DWARFLinker.cpp(383): note: see 
> reference to function template instantiation 'bool std::operator 
> !=(const 
> std::reverse_iterator &,const 
> std::reverse_iterator &)' being compiled
>   C:\Program Files (x86)\Microsoft Visual 
> Studio\2019\Professional\VC\Tools\MSVC\14.28.29333\include\xutility(2086): 
> error C2039: '_Get_current': is not a member of 
> 'std::reverse_iterator'
>   E:\llvm\llvm-project\llvm\include\llvm/DebugInfo/DWARF/DWARFDie.h(405): 
> note: see declaration of 'std::reverse_iterator'

Just saw that you fixed it already. Thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78938

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

[PATCH] D78938: Make LLVM build in C++20 mode

[Nuno Lopes 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 rG92310454bf0f: Make LLVM build in C++20 mode (authored by 
BRevzin, committed by nlopes).

Changed prior to commit:
  https://reviews.llvm.org/D78938?vs=290023=312420#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78938

Files:
  clang/include/clang/AST/StmtIterator.h
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
  llvm/include/llvm/ADT/AllocatorList.h
  llvm/include/llvm/ADT/DenseMap.h
  llvm/include/llvm/ADT/DenseSet.h
  llvm/include/llvm/ADT/DirectedGraph.h
  llvm/include/llvm/ADT/STLExtras.h
  llvm/include/llvm/ADT/StringMap.h
  llvm/include/llvm/ADT/iterator.h
  llvm/include/llvm/CodeGen/DIE.h
  llvm/include/llvm/CodeGen/LiveInterval.h
  llvm/include/llvm/DebugInfo/DWARF/DWARFDie.h
  llvm/include/llvm/DebugInfo/DWARF/DWARFExpression.h
  llvm/include/llvm/IR/Attributes.h
  llvm/include/llvm/IR/BasicBlock.h
  llvm/include/llvm/Object/StackMapParser.h
  llvm/include/llvm/ProfileData/Coverage/CoverageMappingReader.h
  llvm/include/llvm/ProfileData/InstrProfReader.h
  llvm/include/llvm/Support/BinaryStreamRef.h
  llvm/include/llvm/Support/SuffixTree.h
  llvm/lib/CodeGen/PeepholeOptimizer.cpp
  llvm/lib/IR/Attributes.cpp
  llvm/lib/ObjectYAML/DWARFEmitter.cpp
  llvm/lib/Transforms/Scalar/GVNHoist.cpp
  llvm/tools/llvm-objdump/llvm-objdump.cpp
  llvm/unittests/ADT/STLExtrasTest.cpp

Index: llvm/unittests/ADT/STLExtrasTest.cpp
===
--- llvm/unittests/ADT/STLExtrasTest.cpp
+++ llvm/unittests/ADT/STLExtrasTest.cpp
@@ -472,21 +472,21 @@
 
   // Check fancy pointer overload for unique_ptr
   std::unique_ptr V2 = std::make_unique(0);
-  EXPECT_EQ(V2.get(), to_address(V2));
+  EXPECT_EQ(V2.get(), llvm::to_address(V2));
 
   V2.reset(V1);
-  EXPECT_EQ(V1, to_address(V2));
+  EXPECT_EQ(V1, llvm::to_address(V2));
   V2.release();
 
   // Check fancy pointer overload for shared_ptr
   std::shared_ptr V3 = std::make_shared(0);
   std::shared_ptr V4 = V3;
   EXPECT_EQ(V3.get(), V4.get());
-  EXPECT_EQ(V3.get(), to_address(V3));
-  EXPECT_EQ(V4.get(), to_address(V4));
+  EXPECT_EQ(V3.get(), llvm::to_address(V3));
+  EXPECT_EQ(V4.get(), llvm::to_address(V4));
 
   V3.reset(V1);
-  EXPECT_EQ(V1, to_address(V3));
+  EXPECT_EQ(V1, llvm::to_address(V3));
 }
 
 TEST(STLExtrasTest, partition_point) {
Index: llvm/tools/llvm-objdump/llvm-objdump.cpp
===
--- llvm/tools/llvm-objdump/llvm-objdump.cpp
+++ llvm/tools/llvm-objdump/llvm-objdump.cpp
@@ -806,19 +806,19 @@
 bool IsASCII = DbgVariables == DVASCII;
 switch (C) {
 case LineChar::RangeStart:
-  return IsASCII ? "^" : u8"\u2548";
+  return IsASCII ? "^" : (const char *)u8"\u2548";
 case LineChar::RangeMid:
-  return IsASCII ? "|" : u8"\u2503";
+  return IsASCII ? "|" : (const char *)u8"\u2503";
 case LineChar::RangeEnd:
-  return IsASCII ? "v" : u8"\u253b";
+  return IsASCII ? "v" : (const char *)u8"\u253b";
 case LineChar::LabelVert:
-  return IsASCII ? "|" : u8"\u2502";
+  return IsASCII ? "|" : (const char *)u8"\u2502";
 case LineChar::LabelCornerNew:
-  return IsASCII ? "/" : u8"\u250c";
+  return IsASCII ? "/" : (const char *)u8"\u250c";
 case LineChar::LabelCornerActive:
-  return IsASCII ? "|" : u8"\u2520";
+  return IsASCII ? "|" : (const char *)u8"\u2520";
 case LineChar::LabelHoriz:
-  return IsASCII ? "-" : u8"\u2500";
+  return IsASCII ? "-" : (const char *)u8"\u2500";
 }
 llvm_unreachable("Unhandled LineChar enum");
   }
Index: llvm/lib/Transforms/Scalar/GVNHoist.cpp
===
--- llvm/lib/Transforms/Scalar/GVNHoist.cpp
+++ llvm/lib/Transforms/Scalar/GVNHoist.cpp
@@ -149,8 +149,8 @@
   // The instruction (VN) which uses the values flowing out of CHI.
   Instruction *I;
 
-  bool operator==(const CHIArg ) { return VN == A.VN; }
-  bool operator!=(const CHIArg ) { return !(*this == A); }
+  bool operator==(const CHIArg ) const { return VN == A.VN; }
+  bool operator!=(const CHIArg ) const { return !(*this == A); }
 };
 
 using CHIIt = SmallVectorImpl::iterator;
Index: llvm/lib/ObjectYAML/DWARFEmitter.cpp
===
--- llvm/lib/ObjectYAML/DWARFEmitter.cpp
+++ llvm/lib/ObjectYAML/DWARFEmitter.cpp
@@ -650,7 +650,7 @@
 writeInteger((uint8_t)TableEntry.SegSelectorSize, OS, DI.IsLittleEndian);
 
 for (const SegAddrPair  : TableEntry.SegAddrPairs) {
-  if (TableEntry.SegSelectorSize != 0)
+  if (TableEntry.SegSelectorSize != yaml::Hex8{0})
 if (Error Err = writeVariableSizedInteger(Pair.Segment,

[PATCH] D78938: Make LLVM build in C++20 mode

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

Thanks @lebedev.ri for the pointer!
I started working on exactly the same thing as I was trying to link a C++20 
project with LLVM.
@BRevzin is there anything missing in this patch? Do you have commit access or 
do you need help to land this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78938

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

[PATCH] D91055: [clang-tidy] Introduce misc No Integer To Pointer Cast check

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

Nice!
BTW, another popular idiom is to store data in the last few bits of the pointer 
(e.g., LLVM's own PointerIntPair). I guess that one can also be implement by 
casting the ptr to char* and doing operations over that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91055

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

[PATCH] D88979: [InstCombine] combineLoadToOperationType(): don't fold int<->ptr cast into load

[Nuno Lopes via Phabricator via cfe-commits]

nlopes accepted this revision.
nlopes added a comment.

In D88979#2323940 , @lebedev.ri wrote:

> In D88979#2323935 , @nikic wrote:
>
>> LGTM
>
> @nlopes does this look good to you?
>
>> Looking through other uses of isNoopCast(), I don't think it makes sense to 
>> push this change into it, as many other usages do need it to work with 
>> ptrtoint/inttoptr (some of them using it specifically for them). The comment 
>> above the function indicates that "no-op" is to be understood as "generates 
>> no code" here. Possibly it could do with a rename.
>
> I think i don't agree with you there.
> I agree with @nlopes, the end goal will be to basically disallow fusing of 
> `inttoptr`/`ptrtoint` into loads, 
> disallow dropping inttoptr-of-ptrtoint/ptrtoint-of-inttoptr, etc.
> And all that eventually boils down to updating 
> `CastInst::isNoopCast()`/`CastInst::isEliminableCastPair()`.

I'm ok with this change, and I agree it's a good step forwards. Thanks!
My only concern was that it seems this code will need further changes going 
forward (maybe even revert this change?) and I wouldn't want us to forget to 
revisit this code if/when needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88979

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

[PATCH] D88979: [InstCombine] combineLoadToOperationType(): don't fold int<->ptr cast into load

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added inline comments.



Comment at: llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:563
 if (auto* CI = dyn_cast(LI.user_back()))
-  if (CI->isNoopCast(DL))
+  if (CI->isNoopCast(DL) && LI.getType()->isPtrOrPtrVectorTy() ==
+CI->getDestTy()->isPtrOrPtrVectorTy())

What about fixing isNoopCast() directly?
Is the impact too bad/large?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88979

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

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

FYI InstSimplify doing distribution of undef is a known bug: 
https://bugs.llvm.org/show_bug.cgi?id=33165


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83360



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

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a subscriber: tgt.
nlopes added a comment.

In D83360#2142457 , @efriedma wrote:

> > that's fine but I still don't understand why the counterexample to my 
> > version says %x2 in @src can be undef
>
> If I'm understanding correctly, this reduces to something like the following:
>
> define i32 @src() {
>
>   %x2 = freeze i32 undef
>   ret i32 %x2
>
> }
>
> define i32 @tgt() {
>
>   ret i32 undef
>
> }
>
> This seems a little suspect, yes.


This is a known bug: https://github.com/AliveToolkit/alive2/issues/3
gotta fix this soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83360



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

[PATCH] D83360: [InstSimplify] Remove select ?, undef, X -> X and select ?, X, undef -> X

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

In D83360#2140241 , @craig.topper 
wrote:

> Alive does like this https://alive2.llvm.org/ce/z/yhibbe which is what I was 
> going to implement.


right. There's a guaranteedNonPoison (or similar name) in ValueTracking that 
can be used I guess.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83360



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

[PATCH] D81678: Introduce frozen attribute at call sites for stricter poison analysis

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

I'm a bit concerned with this patch as it increases the amount of UB that LLVM 
exploits without any study of the impact.
For example, right now it's ok do this with clang (not with constants; make it 
less trivial so clang doesn't fold it right away):

  int f() { return INT_MAX + 1; }

While technically this is UB in C, when lowered to LLVM IR, this function 
returns poison.
When the frozen attribute is attached, the function will now trigger UB in LLVM 
IR as well.
Is this what we want? It would be worthwhile to at least compile a few programs 
to check if they break. Also, what's the plan to detect these cases in ubsan? 
We shouldn't be exploiting new UB without having good warnings/checks in place 
IMO.

Note that pure function calls with 'frozen' arguments become harder to hoist 
from loops, for example. Since now calling this pure function can trigger UB if 
one of the arguments is poison. You would need to introduce frozen beforehand. 
I don't see this issue addressed in this patch.

For msan & friends, this patch makes sense, as they operate in a rely-guarantee 
way. Initialization of memory is checked, so it can be relied upon by the 
compiler later on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81678



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

[PATCH] D74935: [LangRef][AliasAnalysis] Clarify `noalias` affects only modified objects

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

I agree with @aqjune that stating clearly the definition of object in this 
context.
See this example in the C spec:

  For example, the second call of f in g has undefined behavior because each of 
d[1] through d[49] is accessed through both p and q.
  
  void g(void) {
  extern int d[100];
  f(50, d + 50, d); // valid
  f(50, d + 1, d); // undefined behavior
  }

Restrict just guarantees that accesses are to disjoint memory locations, not 
that they point at different objects (in the sense of they were allocated 
separately).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74935



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

[PATCH] D71499: Add builtins for aligning and checking alignment of pointers and integers

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

@lebedev.ri I agree with you that the semantics of these alignment builtins 
should only return a pointer that is of the same object as the one given as 
input.
Otherwise, these builtins would be even worst that ptr2int/int2ptr, since their 
result could alias with any other pointer in the program, not just the escaped 
pointers.

I gave a glance through the patch, especially the documentation section, and 
the semantics look right to me.
Would be nice to see this using ptrmask on a subsequent patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71499



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

[PATCH] D28952: [analyzer] Add new Z3 constraint manager backend

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

Let me give just 2 more Z3-related suggestions:

- instead of re-creating the solver, it might be faster to do Z3_solver_reset
- "once in a while" it might be helpful to delete everything (all solvers, 
asts, context) and call Z3_reset_memory.  Z3's small object pool is not very 
good and keeps growing /ad infinitum/, so cleaning it up once a while is a good 
thing (improves cache usage and reduces memory consumption).

BTW, you may want to call Z3_finalize_memory at exit to keep clang 
valgrind/asan-clean.  (Z3 keeps a lot of internal buffers otherwise)


https://reviews.llvm.org/D28952



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

[PATCH] D28952: [analyzer] Add new Z3 constraint manager backend

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

Regarding incremental solving with Z3 (or with most SMT solvers in general), 
let me just lower the expectations a bit:
In Z3, when you do push(), there are a few things that change immediately: 1) 
it uses a different SAT solver (one that supports incremental reasoning), and 
2) some preprocessing steps are disabled completely.
The advantage of incremental solving is that it shares the state of the SAT 
solver between queries. On the other hand, Z3 disables a bunch of nice 
preprocessors, and also it switches to eager bit-blasting.  So whether going 
incremental pays off, it depends..  I've seen cases where it's faster to create 
a new solver for each query and cases where incremental is a good thing.
It's on our plans to "fix" Z3 to do better preprocessing in incremental mode, 
but there's no ETA at the moment to start this work..


https://reviews.llvm.org/D28952



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

[PATCH] D28952: [analyzer] Add new Z3 constraint manager backend

[Nuno Lopes via Phabricator via cfe-commits]

nlopes added a comment.

Cool work Dominic!
Just a few random comments from the peanut gallery regarding the usage of Z3:

- I would definitely split the Z3Expr into expr/solver/model classes. Several 
advantages: plugging in new solvers becomes easier and reference counting can 
be handled more safely and automatically. Right now your solver+model related 
code handled ref-counting "by hand", which can easily lead to bugs.
- I would add an rvalue constructor to the expr class to avoid ref-counting 
when unneeded.
- Function z3Expr::fromData() seems to be able to create arbitrarily-sized 
bit-vectors.  I would limit to e.g. 512 bits or something like that.  Z3 is 
super slow with huge bit-vectors. If you feel fancy (for a following version of 
this feature), you could encode this information lazily (based on the model).
- Z3Expr::fromInt() -- why take a string as input and not a uin64_t?
- Why call simplify before asserting a constraint?  That's usually a pretty 
slow thing to do (in my experience).
- You should replace Z3_mk_solver() with something fancier.  If you don't feel 
fancy, just use Z3_mk_simple_solver()  (simple is short for simplifying btw).  
If you know the theory of the constraints (e.g., without floats, it would be 
QF_BV), you should use Z3_mk_solver_for_theory()  (or whatever the name was).  
If you feel really fancy, you could roll your own tactic (in my tools I often 
have a tactic class to build my own tactics; Z3 even allows you to case split 
on the theory after simplifications)
- Instead of asserting multiple constraints to the solver, I would and() them 
and assert just once (plays nicer with some preprocessors of Z3)
- Timeout: SMT solvers sometimes go crazy and take forever.  I would advise to 
add a timeout to the solver to avoid getting caught on a trap.  This timeout 
can be different for the two use cases: check if a path is feasible (lower 
timeout), or check if you actually have a bug (higher timeout)
- Also, I didn't see anything regarding memory (I might have missed yet).  Are 
you using the array theory?  If so, I would probably advise benchmarking it 
carefully since Z3's support for arrays is not great (even though it has 
improved a bit in the last year or so).

BTW, adding small amount of constant folding in the expression builder is 
usually good, since you save time in Z3 preprocessor. Especially if you want to 
go for smallish timeouts (likely).  If you want to be fancy, you can also 
canonicalize expressions like Z3 likes (e.g., only ule/sle, no slt, sgt, etc).


https://reviews.llvm.org/D28952



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