[PATCH] D85345: [BuildLibCalls] Add noundef to standard I/O functions

aqjune updated this revision to Diff 284246. aqjune added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. Update PR3589-freestanding-libcalls.c Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85345/new/ https://revie

[PATCH] D85345: [BuildLibCalls] Add noundef to standard I/O functions

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGef018cb65c98: [BuildLibCalls] Add noundef to standard I/O functions (authored by aqjune). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D85345: [BuildLibCalls] Add noundef to standard I/O functions

aqjune added a comment. If things go well, I'll add noundef to other library functions such as utime, setenv, unsetenv, as well. For malloc/calloc/realloc/free, I'll think more about them. I guess it is safe to tag noundef to free's operand, but for malloc I'm not 100% sure... Repository:

[PATCH] D85345: [BuildLibCalls] Add noundef to standard I/O functions

aqjune added a comment. What about undef or poison is given to malloc? If it should raise UB, the size argument and returned pointer should be noundef. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85345/new/ https://reviews.llvm.org/D85345 _

[PATCH] D85345: [BuildLibCalls] Add noundef to standard I/O functions

aqjune added a comment. In D85345#2205745 , @jdoerfert wrote: > In D85345#2205712 , @aqjune wrote: > >> What about undef or poison is given to malloc? If it should raise UB, the >> size argument and returned pointe

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

aqjune added a comment. Seems like a bug in instsimplify: define i1 @f(i32 %x, i32 %y) { %cmp9.not.1 = icmp eq i32 %x, %y %cmp15 = icmp slt i32 %x, %y %spec.select39 = select i1 %cmp9.not.1, i1 undef, i1 %cmp15 %spec.select40 = xor i1 %cmp9.not.1, 1 %spec.select = and

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

aqjune added a comment. (renaming variables for readability) %a = select i1 %s, i1 undef, i1 %t %b = xor i1 %s, 1 %c = and i1 %a, %b This series of reasoning happened from a single SimplifyAndInst call: c = a & (s ^ 1) = (a & s) ^ (a & 1); ExpandBinOp = ((sel

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

aqjune added a comment. In D81678#2091089 , @eugenis wrote: > Positive attribute sounds good to me (frozen is not a bad name), but the > tests update change is going to be huge. Any concerns about IR size bloat? > The attribute will apply to the majority

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

aqjune accepted this revision. aqjune added a comment. This revision is now accepted and ready to land. For the LangRef part, LGTM modulo the suggested change. Could anyone review clang and LLVM's updated part? Comment at: llvm/docs/LangRef.rst:1644 +and return values. Whe

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

aqjune added a comment. In D81678#2099444 , @efriedma wrote: > So I guess we've discussed the following alternatives so far: > > (snip) > > Maybe (1) is the least-bad; all the others compromise by making LLVM harder > to understand. We can make porting t

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

aqjune added a comment. In D81678#2105077 , @eugenis wrote: > > Could you explicitly state that if it is aggregate or vector type all > > elements and paddings should be frozen too? > > If an aggregate is passed as an aggregate at IR level, we should not

[PATCH] D79636: [LangRef] Clarify the semantics of the `byval` attribute

aqjune added a comment. I have a minor question: > a call of a readnone function with a byval argument is not classified as > readnone (which it is today: https://godbolt.org/z/dDfQ5r) %0 at caller has readnone attribute - is it related with the propagation of readnone attribute from %0 of emp

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

aqjune added a comment. In D83360#2154584 , @echristo wrote: > It's that even before the msan instrumentation the IR doesn't look correct - > thus a miscompile. I'll share a prototype of the InstSimplify patch on Phabricator, in a day or two; it would

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

aqjune added a comment. In D83360#2162898 , @craig.topper wrote: > @aqjune did you put a patch for InstSimplify doing distribution over undef > yet? Sorry, making InstSimplify to safely distribute undef was a nontrivial job - other than updating InstS

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

aqjune added a comment. Made a patch at https://reviews.llvm.org/D84250 . 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-commi

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

aqjune added a comment. Hi, this is really interesting. I was interested in statically analyzing whether a value is undef/poison, so I also thought about the existence of this attribute, but I never imagined that MSan would benefit from this attribute as well. > The partialinit attribute is, i

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

aqjune added a comment. Hi, The term 'object' seems ambiguous to me. For example, void f(i32* noalias x, i32* noalias y) { *x = 1; *y = 2; } int x[2]; f(&x[0], &x[1]); If object means an allocation, this should be UB, because x and y are pointing to the same object. Memory lo

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

aqjune added a comment. Never mind, I see that the keyword object comes from the alias analysis document. The alias analysis document is using the term object consistently. I was confused because the term is used at llvm.objectsize intrinsic's LangRef documentation, which seems to refer an alloc

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

aqjune added a comment. I agree readnone functions can read constant memory; at least LangRef doesn't restrict readnone functions from accessing memory (unless it changes a state visible to caller). I see that readnone is also attached to a function that loads from/stores to alloca: https://god

[PATCH] D92270: [ConstantFold] Fold more operations to poison

aqjune added a comment. In D92270#2538713 , @RKSimon wrote: > https://bugs.llvm.org/show_bug.cgi?id=49005 seems to be due to this (either > directly or it has unearthed an existing problem) I reverted this commit; I'll reland this after the underlying pr

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

aqjune added a comment. I'm fine with any direction that helps people land test updates/implementations. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81678 ___ cfe-c

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

aqjune added a comment. Hi, Naming is a hard thing... I have no special preference. :/ However, I'd like to understand the details of this attribute. Would LTO be affected because `leaf` is guaranteed to untouch the current translation unit only? // a.c int x; void f1() { f2(); }

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

aqjune added a comment. In D90275#2371813 , @jdoerfert wrote: > As noted by the GCC docs, it doesn't mean anything on a definition so that > you can safely merge TUs. I want us to forbid `leaf` on IR function > definitions for that reason, it would not m

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

aqjune added a comment. In D90275#2374648 , @gulfem wrote: > Leaf attribute is specifically intended for library functions and I think all > the existing usage of leaf attribute is in the library function declarations. > For ex, it is only used in syscall

[PATCH] D82317: [Clang/Test]: Update tests where `noundef` attribute is necessary

aqjune added a comment. Herald added a subscriber: frasercrmck. In D82317#2201215 , @rjmccall wrote: > second, it's yet another contribution towards the giant pile of attributes > that seem to have become necessary to do any work in LLVM I don't think th

[PATCH] D94380: [InstCombine] Update transformations to use poison for insertelement/shufflevector's placeholder value (1/2)

aqjune added inline comments. Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:1728 // Vec. if (IdxN % DstNumElts != 0 || IdxN + DstNumElts > VecNumElts) { replaceInstUsesWith(CI, UndefValue::get(CI.getType())); Guarded by t

[PATCH] D93817: [InstCombine] Update transformations to use poison for insertelement/shufflevector's placeholder value (2/2)

aqjune added inline comments. Comment at: llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:1221 if (!isa(I->getOperand(1))) { -I->setOperand(1, UndefValue::get(I->getOperand(1)->getType())); MadeChange = true; This change is

[PATCH] D86841: [clang] Add mustprogress and llvm.loop.mustprogress attribute deduction

aqjune added inline comments. Comment at: clang/lib/CodeGen/CGStmt.cpp:801 + getLangOpts().CPlusPlus11 || getLangOpts().CPlusPlus14 || + getLangOpts().CPlusPlus17 || getLangOpts().C2x) { +LoopMustProgress = true; A silly question: does

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

aqjune added a comment. I think it makes sense - IIUC, for most of the clang tests, noundef won't be the attribute of interest. For brevity of tests, I think the change is fine. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://revi

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

aqjune added a comment. +1 for this patch! 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.l

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

aqjune added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/misc-no-inttoptr.rst:12 +if you got that integral value via a pointer-to-integer cast originally, +the new pointer will lack the provenance information from the original pointer. +

[PATCH] D92270: [ConstantFold] Fold more operations to poison

aqjune updated this revision to Diff 308200. aqjune added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. update clang/test/Frontend/fixed_point_unary.c It seems unsigned _Fract type can only represent [0.0, 1.0)? I tried to find a relevant sentence from http:/

[PATCH] D92270: [ConstantFold] Fold more operations to poison

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG53040a968dc2: [ConstantFold] Fold more operations to poison (authored by aqjune). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D92270: [ConstantFold] Fold more operations to poison

aqjune added a comment. Hi, It seems it is related to two optimizations: (1) `select i1 x, true, y` -> `or i1 x, y` (2) `or i1 x, poison` -> `poison` Semantically, the first one is broken. It needs to freeze y. But, it will introduce a lot of freeze instructions. The clang patches that introduc

[PATCH] D92270: [ConstantFold] Fold more operations to poison

aqjune added a comment. In D92270#2422661 , @uabelho wrote: > Should langref also be updated to describe this? Or does it already? > I just found this > "An instruction that depends on a poison value, produces a poison value > itself." > here > http://l

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

aqjune added a comment. Hello all, What is the status of this patch? Do we need someone who look into the lit change? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81678/new/ https://reviews.llvm.org/D81678

[PATCH] D85788: [Clang test] Update to allow passing extra default clang arguments in use_clang

aqjune added a comment. Herald added a subscriber: frasercrmck. In D85788#2335838 , @eugenis wrote: > I wonder it can be avoided by > > - disable noundef analysis by default in cc1 > - always add -enable-noundef-analysis in the driver when invoking cc1 Wo

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

aqjune added a comment. In D81678#2438264 , @eugenis wrote: > This change looks fine to me, but I'm slightly concerned about > https://reviews.llvm.org/D85788 - see my last comment on that revision. Thank you for the link! I left a comment there. Repos

[PATCH] D85788: [Clang test] Update to allow passing extra default clang arguments in use_clang

aqjune added a comment. In D85788#2444136 , @guiand wrote: > IMO it's better to just one-and-done programatically add `-Xclang > -disable-noundef-analysis` to all the tests' RUN lines. That way there are no > hidden flags. If a script for this can be wr

[PATCH] D93586: [InstCombine] use poison as placeholder for undemanded elems

aqjune added a comment. In D93586#2464841 , @spatel wrote: > Besides changing IRBuilder and shufflevector's definition, I think we'll also > need updates in the vectorizers, InstSimplify, and other places in > InstCombine that use UndefValue for InsertEl

[PATCH] D93586: [InstCombine] use poison as placeholder for undemanded elems

aqjune added a comment. In D93586#2466284 , @aqjune wrote: > The transition will be swiftly done (if there's no other issue hopefully) by > the next weekend. Oops, I meant this weekend hopefully. Repository: rG LLVM Github Monorepo CHANGES SINCE LAS

[PATCH] D93586: [InstCombine] use poison as placeholder for undemanded elems

aqjune added a comment. > There are 792 files in llvm/test, most of which are in test/Transform (119) > and test/CodeGen(655). > The transition will be swiftly done (if there's no other issue hopefully) by > the next weekend. Thinking about these again, do we need to make a poison copy for test

[PATCH] D93586: [InstCombine] use poison as placeholder for undemanded elems

aqjune added a comment. In D93586#2467844 , @RKSimon wrote: > I'm sorry I've only just started looking at this - are you saying that you > want to handle all "insertelement undef" cases in one go and not just a > series of patcches after this one? It wi

[PATCH] D93586: [InstCombine] use poison as placeholder for undemanded elems

aqjune added a comment. In D93586#2468350 , @spatel wrote: > It would be good to update those for consistency; the codegen tests are > supposed to be representative of what comes out of the IR optimizer. IIUC, we > could do the substitution on those file

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

aqjune created this revision. aqjune added reviewers: spatel, lebedev.ri, efriedma, nlopes, regehr, RKSimon, zhengyangl, nikic, hfinkel. Herald added subscribers: dmgreen, kbarton, hiraditya, nemanjai. aqjune requested review of this revision. Herald added projects: clang, LLVM. Herald added subsc

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

aqjune 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"); } nlopes wrote: > while at it, don't you want to c

[PATCH] D93586: [InstCombine] use poison as placeholder for undemanded elems

aqjune added a comment. I'll make two more patches - the instsimplify/vectorizer/ changes that make insertelement poison, and the langref update to shufflevector. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93586/new/ https://reviews.llvm.or

[PATCH] D93586: [InstCombine] use poison as placeholder for undemanded elems

aqjune added a comment. There are 3 more patches: https://reviews.llvm.org/D93793 (IRBuilder's CreateVectorSplat) https://reviews.llvm.org/D93817 (Other transformations) https://reviews.llvm.org/D93818 (LangRef) Would it be desirable if I land all of these at once as well as this (93586) when

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

aqjune 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"); } nikic wrote: > aqjune wrote: > > nlopes wrote: >

[PATCH] D93586: [InstCombine] use poison as placeholder for undemanded elems

aqjune added a comment. Okay, I'll gently land this. I was just wondering whether insertelements having heterogenous placeholder would be problematic (this patch makes some of insertelement use poison, but not all), but it may not matter very much. Repository: rG LLVM Github Monorepo CHANGE

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

aqjune 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"); } aqjune wrote: > nikic wrote: > > aqjune wrote: >

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

aqjune updated this revision to Diff 313945. aqjune added a comment. Herald added subscribers: kerbowa, nhaehnle, jvesely. Update IRBuilder to fill poison at shufflevector's empty operand Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93793/new/ htt

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

aqjune added inline comments. Comment at: clang/test/CodeGen/SystemZ/builtins-systemz-zvector-constrained.c:84 vd = vec_splat(vd, 1); // CHECK: shufflevector <2 x double> %{{.*}}, <2 x double> undef, <2 x i32> // CHECK-ASM: vrepg These SystemZ zvector

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

aqjune updated this revision to Diff 313989. aqjune added a comment. Herald added a reviewer: bollu. Update comments Call unary CreateShuffleVector Update polly tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93793/new/ https://reviews.llvm.org

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

aqjune marked an inline comment as done. aqjune added inline comments. Comment at: llvm/lib/IR/IRBuilder.cpp:1020 Value *Zeros = ConstantAggregateZero::get(VectorType::get(I32Ty, EC)); - return CreateShuffleVector(V, Undef, Zeros, Name + ".splat"); + return CreateShuffleVect

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. aqjune marked an inline comment as done. Closed by commit rG278aa65cc495: [IR] Let IRBuilder's CreateVectorSplat/CreateShuffleVector use poison as… (authored by aqjune)

[PATCH] D93923: Use unary CreateShuffleVector if possible

aqjune created this revision. aqjune added reviewers: spatel, nikic, lebedev.ri, craig.topper, RKSimon, efriedma. Herald added subscribers: dexonsmith, kerbowa, pengfei, dmgreen, hiraditya, nhaehnle, jvesely, nemanjai, arsenm. aqjune requested review of this revision. Herald added projects: clang

[PATCH] D93923: Use unary CreateShuffleVector if possible

aqjune added inline comments. Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:2260 - V = IRB.CreateSelect(ConstantVector::get(Mask), V, Old, Name + "blend"); + V = IRB.CreateSelect(ConstantVector::get(Mask2), V, Old, Name + "blend"); LLVM_DEBUG(dbgs() << "blend: " << *

[PATCH] D93923: Use unary CreateShuffleVector if possible

aqjune added a comment. Thanks! :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93923/new/ https://reviews.llvm.org/D93923 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/

[PATCH] D93923: Use unary CreateShuffleVector if possible

aqjune updated this revision to Diff 314108. aqjune added a comment. - clang-format, address warnings Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93923/new/ https://reviews.llvm.org/D93923 Files: clang/lib/CodeGen/CGBuiltin.cpp clang/lib/Cod

[PATCH] D93923: Use unary CreateShuffleVector if possible

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG9b29610228c8: Use unary CreateShuffleVector if possible (authored by aqjune). Changed prior to commit: https://reviews.llvm.org/D93923?vs=314108&i

[PATCH] D93817: Update transformations to use poison for new insertelement's placeholder value

aqjune updated this revision to Diff 314123. aqjune added a comment. Herald added subscribers: cfe-commits, pengfei, aheejin, jgravelle-google, sbc100, dschuff. Herald added a project: clang. Rebase, update transformations with shufflevector involved too Repository: rG LLVM Github Monorepo C

[PATCH] D93817: Update transformations to use poison for insertelement/shufflevector's placeholder value

aqjune added a comment. I'll share the result after checking the validity of this patch using alive2. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93817/new/ https://reviews.llvm.org/D93817 ___ cfe-comm

[PATCH] D93817: Update transformations to use poison for insertelement/shufflevector's placeholder value

aqjune updated this revision to Diff 314175. aqjune added a comment. Find a few missing places & update them to use poison Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93817/new/ https://reviews.llvm.org/D93817 Files: clang/test/CodeGen/SystemZ

[PATCH] D93817: Update transformations to use poison for insertelement/shufflevector's placeholder value

aqjune added a comment. Running Alive2 finds one failure, which is fixed by updating the shufflevector semantics to return poison on undef/poison mask (D93818 ): Transforms/InstCombine/shufflevector-div-rem-inseltpoison.ll

[PATCH] D93817: Update transformations to use poison for insertelement/shufflevector's placeholder value

aqjune added a comment. I'll split this patch into smaller pieces, so they're able to get reviewed more easily. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93817/new/ https://reviews.llvm.org/D93817 _

[PATCH] D93817: Update transformations to use poison for insertelement/shufflevector's placeholder value

aqjune updated this revision to Diff 314289. aqjune added a comment. Rebase I'll continue splitting after working on lifetime patches Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93817/new/ https://reviews.llvm.org/D93817 Files: clang/test/Code

[PATCH] D92270: [ConstantFold] Fold more operations to poison

aqjune added a comment. In D92270#2482741 , @thakis wrote: > We bisected a test failure to this > (https://bugs.chromium.org/p/angleproject/issues/detail?id=5500#c17). Can you > expand a bit on what this patch means in practice? I'm guessing it makes UB

[PATCH] D92270: [ConstantFold] Fold more operations to poison

aqjune added a comment. To fix the old bug quite a few patches in LLVM have landed so far and it is still ongoing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92270/new/ https://reviews.llvm.org/D92270 __

[PATCH] D92270: [ConstantFold] Fold more operations to poison

aqjune added a comment. In D92270#2483557 , @thakis wrote: > It turned out to be UB in our code as far as I can tell, see > https://bugs.chromium.org/p/angleproject/issues/detail?id=5500#c36 and the > follow-on. > > (If this is known to trigger an existi

[PATCH] D103874: [IR] Rename the shufflevector's undef mask to poison

aqjune created this revision. aqjune added reviewers: nikic, efriedma, spatel, fhahn, lebedev.ri, RKSimon. Herald added a reviewer: deadalnix. Herald added subscribers: dexonsmith, kerbowa, pengfei, dmgreen, zzheng, kbarton, hiraditya, nhaehnle, jvesely, nemanjai. aqjune requested review of this r

[PATCH] D103874: [IR] Rename the shufflevector's undef mask to poison

aqjune added a comment. In D103874#2806519 , @efriedma wrote: > I noted the cases where it looks like the undef->poison change might actually > impact code using compiler intrinsic functions that have external > specifications. The relevant specificati

[PATCH] D104790: [x86] fix mm*_undefined* intrinsics to use arbitrary frozen bit pattern

aqjune created this revision. aqjune added reviewers: efriedma, spatel, craig.topper, RKSimon. Herald added a subscriber: pengfei. aqjune requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This fixes lowering of `mm*_undefined*` intrinsics to

[PATCH] D104790: [x86] fix mm*_undefined* intrinsics to use arbitrary frozen bit pattern

aqjune added a comment. I couldn't find end-to-end tests for checking assembly generation. To check whether this is working ok, which tests should I write and how would it look like? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104790/new/ https:

[PATCH] D82317: [Clang/Test]: Update tests where `noundef` attribute is necessary

aqjune added a comment. Thank you all! Incremental change makes sense to me as well. It will help smooth landing without buildbot failures. I'll start writing patches soon. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82317/new/ https://reviews.l

[PATCH] D104790: [x86] fix mm*_undefined* intrinsics to use arbitrary frozen bit pattern

aqjune updated this revision to Diff 354675. aqjune added a comment. Herald added subscribers: llvm-commits, ecnelises, hiraditya. Herald added a project: LLVM. - Update llvm's fast-isels tests for undefined intrinsics to compile freeze(poison) - Update X86ISelLowering's getAVX2GatherNode and get

[PATCH] D104790: [x86] fix mm*_undefined* intrinsics to use arbitrary frozen bit pattern

aqjune updated this revision to Diff 354678. aqjune added a comment. Minor fixes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104790/new/ https://reviews.llvm.org/D104790 Files: clang/lib/CodeGen/CGBuiltin.cpp clang/test/CodeGen/X86/avx-built

[PATCH] D104790: [x86] fix mm*_undefined* intrinsics to use arbitrary frozen bit pattern

aqjune updated this revision to Diff 354682. aqjune added a comment. - Update llvm/test/CodeGen/X86/sse-intrinsics-fast-isel.ll as well Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104790/new/ https://reviews.llvm.org/D104790 Files: clang/lib/C

[PATCH] D104790: [x86] fix mm*_undefined* intrinsics to use arbitrary frozen bit pattern

aqjune added a comment. In D104790#2836463 , @craig.topper wrote: > In D104790#2836253 , @aqjune wrote: > >> I couldn't find end-to-end tests for checking assembly generation. >> To check whether this is working

[PATCH] D104790: [x86] fix mm*_undefined* intrinsics to use arbitrary frozen bit pattern

aqjune added a comment. In D104790#2842523 , @nikic wrote: > Is this actually better in any meaningful way? InstCombine will turn `freeze > poison` into `zeroinitializer`, and until then this is just a completely > opaque value. I think to correctly em

[PATCH] D82317: [Clang/Test]: Update tests where `noundef` attribute is necessary

aqjune added a comment. I asked @hyeongyukim (who is a participant of google summer of code) to rebase this patch and make an updated version. I thought it was a good goal for him to target, as the validity of the enable-noundef flag is already reviewed and the changes are syntactic updates to

[PATCH] D105169: [Clang/Test]: Enable enable_noundef_analysis by default

aqjune updated this revision to Diff 368033. aqjune added a comment. Rename `disable-noundef-args` to `disable-noundef-analysis` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105169/new/ https://reviews.llvm.org/D105169 Files: clang/include/clan

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

aqjune added a comment. Hello all, I'm a messenger of @hyeongyukim's two patches that allow clang to turn on noundef analysis by default. The noundef analysis flag was added by D81678 in the past. Its goal is to mark arguments and return values in C/C++ as `no

[PATCH] D108453: [Clang/Test]: Enable enable_noundef_analysis as default(2)

aqjune updated this revision to Diff 368034. aqjune added a comment. Rebase, rename `disable-noundef-args` to `disable-noundef-analysis`, remove redundant diffs Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108453/new/ https://reviews.llvm.org/D10

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

aqjune added a comment. Hello all, I'm a messenger of @hyeongyukim's two patches that allow clang to turn on noundef analysis by default. The motivation and background of these patches are described at https://reviews.llvm.org/D105169#2959464. Here is a list of tests that has `RUN: %clang_cc1`

[PATCH] D101191: [InstCombine] Fully disable select to and/or i1 folding

aqjune added inline comments. Comment at: llvm/test/Transforms/InstCombine/logical-select.ll:385 +; CHECK-NEXT:[[OR:%.*]] = select i1 [[AND1]], i1 true, i1 [[AND2]] +; CHECK-NEXT:ret i1 [[OR]] ; nikic wrote: > It looks like this fold could be salvaged, i

[PATCH] D101191: [InstCombine] Fully disable select to and/or i1 folding

aqjune added inline comments. Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2680 +// select c, (select a, true, b), false -> select c, a, false +// if c implies that b is false. +if (match(CondVal, m_Select(m_Value(A), m_One(), m_Value(B))) && -

[PATCH] D101191: [InstCombine] Fully disable select to and/or i1 folding

aqjune added inline comments. Comment at: llvm/test/Transforms/InstCombine/logical-select.ll:385 +; CHECK-NEXT:[[OR:%.*]] = select i1 [[AND1]], i1 true, i1 [[AND2]] +; CHECK-NEXT:ret i1 [[OR]] ; nikic wrote: > aqjune wrote: > > nikic wrote: > > > It look

[PATCH] D101191: [InstCombine] Fully disable select to and/or i1 folding

aqjune added inline comments. Comment at: llvm/test/Transforms/PhaseOrdering/unsigned-multiply-overflow-check.ll:20 +; FIXME: noundef should be attached to args define i1 @will_not_overflow(i64 %arg, i64 %arg1) { aqjune wrote: > spatel wrote: > > Any ideas ab

[PATCH] D101191: [InstCombine] Fully disable select to and/or i1 folding

aqjune added inline comments. Comment at: llvm/test/Transforms/PhaseOrdering/unsigned-multiply-overflow-check.ll:20 +; FIXME: noundef should be attached to args define i1 @will_not_overflow(i64 %arg, i64 %arg1) { nikic wrote: > spatel wrote: > > aqjune wrote:

[PATCH] D101191: [InstCombine] Fully disable select to and/or i1 folding

aqjune added a comment. I found that there are a few more patterns that can be salvaged. Will create a patch for them. Comment at: llvm/test/Transforms/InstCombine/and.ll:898 ; CHECK-NEXT:[[Y:%.*]] = icmp ugt i72 [[C:%.*]], 42 -; CHECK-NEXT:[[AND:%.*]] = and i1 [[X]],

[PATCH] D101191: [InstCombine] Fully disable select to and/or i1 folding

aqjune added a comment. I made D101720 to cover the remaining cases except `Transforms/InstCombine/and2.ll`. Supporting `and2.ll` doesn't seem super-straightforward, but maybe some minor tweaks can make it. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D101191: [InstCombine] Fully disable select to and/or i1 folding

aqjune added a comment. (BTW, if the patch is reviewed, then I believe we are finally ready to land this patch.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101191/new/ https://reviews.llvm.org/D101191 __

[PATCH] D101191: [InstCombine] Fully disable select to and/or i1 folding

aqjune added inline comments. Comment at: llvm/test/Transforms/InstCombine/or.ll:1102 +; CHECK-NEXT:[[AND:%.*]] = select i1 [[Y]], i1 [[X]], i1 false +; CHECK-NEXT:[[OR:%.*]] = select i1 [[X_INV]], i1 true, i1 [[AND]] ; CHECK-NEXT:ret i1 [[OR]] nikic

[PATCH] D101191: [InstCombine] Fully disable select to and/or i1 folding

aqjune added inline comments. Comment at: llvm/test/Transforms/InstCombine/or.ll:1135 ; CHECK-NEXT:ret i1 [[OR]] ; %x = icmp sge i16 %a, %b This can be salvaged as well: https://alive2.llvm.org/ce/z/yXF96T But I think there are more patterns that are m

[PATCH] D101191: [InstCombine] Fully disable select to and/or i1 folding

aqjune added a comment. I think this patch is ready to go: running the test-suite on an ARM machine didn't complain anything. Well, but one thing that I'm concerned about is that from tomorrow I'll not be online for about three weeks. :( I'd like to find someone who reverts this patch if there

[PATCH] D101191: [InstCombine] Fully disable select to and/or i1 folding

aqjune added a comment. In D101191#2738130 , @nikic wrote: > LGTM. I can take care of reverting if there are issues. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101191/new/ https://reviews.llvm.

[PATCH] D82317: [Clang/Test]: Update tests where `noundef` attribute is necessary

aqjune added a comment. Herald added a subscriber: lxfind. Hello all, Is there a plan to enable `-enable-noundef-analysis` by default? It seems this patch is already addressing a lot of test cases. Another good news is that DeadArgElim is also fixed by D98899 to

[PATCH] D103874: [IR] Rename the shufflevector's undef mask to poison

aqjune added a comment. Herald added subscribers: jsji, kosarev. Herald added a project: All. It seems llvm/lib/Target/X86/X86ISelLowering.cpp's LowerAVXCONCAT_VECTORS is relevant to efficient lowering of `shufflevector %x, freeze(poison), mask`. Repository: rG LLVM Github Monorepo CHANGES S

[PATCH] D103874: [IR] Rename the shufflevector's undef mask to poison

aqjune added a comment. In D103874#3594617 , @aqjune wrote: > It seems llvm/lib/Target/X86/X86ISelLowering.cpp's LowerAVXCONCAT_VECTORS is > relevant to efficient lowering of `shufflevector %x, freeze(poison), mask`. After patching `LowerAVXCONCAT_VECTO

  1   2   >