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

2021-02-09 Thread Juneyoung Lee via Phabricator via cfe-commits
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 ___

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

2021-02-03 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2021-01-10 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2021-01-10 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2021-01-07 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2021-01-06 Thread Juneyoung Lee via Phabricator via cfe-commits
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

2021-01-06 Thread Juneyoung Lee via Phabricator via cfe-commits
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] D93817: Update transformations to use poison for insertelement/shufflevector's placeholder value

2021-01-03 Thread Juneyoung Lee via Phabricator via cfe-commits
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:

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

2021-01-02 Thread Juneyoung Lee via Phabricator via cfe-commits
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

2020-12-31 Thread Juneyoung Lee via Phabricator via cfe-commits
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

2020-12-31 Thread Juneyoung Lee via Phabricator via cfe-commits
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:

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

2020-12-30 Thread Juneyoung Lee via Phabricator via cfe-commits
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 ___

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

2020-12-30 Thread Juneyoung Lee via Phabricator via cfe-commits
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

[PATCH] D93923: Use unary CreateShuffleVector if possible

2020-12-30 Thread Juneyoung Lee 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 rG9b29610228c8: Use unary CreateShuffleVector if possible (authored by aqjune). Changed prior to commit:

[PATCH] D93923: Use unary CreateShuffleVector if possible

2020-12-30 Thread Juneyoung Lee via Phabricator via cfe-commits
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

[PATCH] D93923: Use unary CreateShuffleVector if possible

2020-12-30 Thread Juneyoung Lee via Phabricator via cfe-commits
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

[PATCH] D93923: Use unary CreateShuffleVector if possible

2020-12-29 Thread Juneyoung Lee via Phabricator via cfe-commits
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

2020-12-29 Thread Juneyoung Lee via Phabricator via cfe-commits
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:

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

2020-12-29 Thread Juneyoung Lee via Phabricator via cfe-commits
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 IRBuilders CreateVectorSplat/CreateShuffleVector use poison as… (authored by

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

2020-12-29 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-12-29 Thread Juneyoung Lee via Phabricator via cfe-commits
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/

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

2020-12-29 Thread Juneyoung Lee via Phabricator via cfe-commits
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 use poison as inselt's placeholder

2020-12-29 Thread Juneyoung Lee via Phabricator via cfe-commits
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/

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

2020-12-27 Thread Juneyoung Lee via Phabricator via cfe-commits
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] D93586: [InstCombine] use poison as placeholder for undemanded elems

2020-12-27 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-12-27 Thread Juneyoung Lee via Phabricator via cfe-commits
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

2020-12-27 Thread Juneyoung Lee via Phabricator via cfe-commits
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] D93586: [InstCombine] use poison as placeholder for undemanded elems

2020-12-25 Thread Juneyoung Lee via Phabricator via cfe-commits
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/

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

2020-12-25 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-12-23 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-12-23 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-12-22 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-12-21 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-12-21 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-12-21 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-12-11 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-12-08 Thread Juneyoung Lee via Phabricator via cfe-commits
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.

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

2020-12-08 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-12-07 Thread Juneyoung Lee via Phabricator via cfe-commits
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] D92270: [ConstantFold] Fold more operations to poison

2020-11-30 Thread Juneyoung Lee via Phabricator via cfe-commits
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 >

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

2020-11-30 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-11-29 Thread Juneyoung Lee 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 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

2020-11-28 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-11-27 Thread Juneyoung Lee via Phabricator via cfe-commits
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] D82317: [Clang/Test]: Update tests where `noundef` attribute is necessary

2020-11-05 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-11-04 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-11-04 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-11-03 Thread Juneyoung Lee via Phabricator via cfe-commits
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] D88979: [InstCombine] combineLoadToOperationType(): don't fold int<->ptr cast into load

2020-10-11 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-10-08 Thread Juneyoung Lee via Phabricator via cfe-commits
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/

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

2020-09-30 Thread Juneyoung Lee via Phabricator via cfe-commits
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:

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

2020-08-10 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-08-09 Thread Juneyoung Lee via Phabricator via cfe-commits
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

2020-08-09 Thread Juneyoung Lee via Phabricator via cfe-commits
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...

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

2020-08-09 Thread Juneyoung Lee 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 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

2020-08-09 Thread Juneyoung Lee via Phabricator via cfe-commits
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/

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

2020-07-21 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-07-20 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-07-15 Thread Juneyoung Lee via Phabricator via cfe-commits
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

2020-07-12 Thread Juneyoung Lee via Phabricator via cfe-commits
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 =

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

2020-07-11 Thread Juneyoung Lee via Phabricator via cfe-commits
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] D81678: Introduce frozen attribute at call sites for stricter poison analysis

2020-06-21 Thread Juneyoung Lee via Phabricator via cfe-commits
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] D81678: Introduce frozen attribute at call sites for stricter poison analysis

2020-06-20 Thread Juneyoung Lee via Phabricator via cfe-commits
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.

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

2020-06-20 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-06-17 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-06-12 Thread Juneyoung Lee via Phabricator via cfe-commits
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,

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

2020-05-09 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-02-23 Thread Juneyoung Lee via Phabricator via cfe-commits
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:

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

2020-02-20 Thread Juneyoung Lee via Phabricator via cfe-commits
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

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

2020-02-20 Thread Juneyoung Lee via Phabricator via cfe-commits
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([0], [1]); If object means an allocation, this should be UB, because x and y are pointing to the same object. Memory