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

jmciver added a comment. The Discourse proposal is available: RFC Load Instruction: Uninitialized Memory Semantics Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

jmciver added a comment. As Nuno mentioned we are targeting the proposal for next week. I will update the ticket with the Discourse link once it becomes available. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134410/new/

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

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

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

nikic added a comment. In D134410#3898615 , @nlopes wrote: > In D134410#3898563 , @nikic wrote: > >> FWIW, I don't agree with this. It's fine to change load semantics, as long >> as bitcode autoupgrade is

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

jmciver updated this revision to Diff 480265. jmciver added a comment. Updating D134410 : [clang][CodeGen] Add noundef metadata to load instructions (preliminary 1 or 3) The following tests have been updated: - avx512f-builtins.c - avx512fp16-builtins.c -

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

jmciver updated this revision to Diff 478821. jmciver added a comment. Updating D134410 : [clang][CodeGen] Add noundef metadata to load instructions (preliminary 1 or 2) Refactor test matrix-type-operators.c to contain the noundef attribute. This test will be

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

nlopes added a comment. In D134410#3898563 , @nikic wrote: > In D134410#3895253 , @nlopes wrote: > >> In D134410#3895077 , @nikic wrote: >> >>> In D134410#3894995

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

nikic added a comment. In D134410#3895253 , @nlopes wrote: > In D134410#3895077 , @nikic wrote: > >> In D134410#3894995 , @nlopes wrote: >> >>> We wanted this patch to

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

jmciver added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:676 +namespace { +void applyNoundefToLoadInst(bool enable, const clang::QualType , +llvm::LoadInst *Load) { tschuett wrote: > Nit: You meant static. Thank you for

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

jmciver updated this revision to Diff 472051. jmciver added a comment. Updating D134410 : [clang][CodeGen] Add noundef metadata to load instructions (preliminary 1 or 2) Refactor local linkage function, applyNoundefToLoadInst, to use static rather than an

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

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

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

jmciver updated this revision to Diff 471878. jmciver added a comment. Herald added subscribers: kosarev, kerbowa, jvesely. Updating D134410 : [clang][CodeGen] Add noundef metadata to load instructions (preliminary 1 or 2) Fix AMD-GPU, ARM, PowerPC, and new

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

tschuett added a comment. Almost all flags are off by default. That's why they are called flags, i.e., `-Weverything`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134410/new/ https://reviews.llvm.org/D134410

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

tschuett added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:676 +namespace { +void applyNoundefToLoadInst(bool enable, const clang::QualType , +llvm::LoadInst *Load) { Nit: You meant static. Repository: rG LLVM Github

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

nikic added a comment. 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

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

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

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

nikic added a comment. 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. Repository: rG LLVM

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

jmciver added a comment. I'll start fixing reported test failures tomorrow. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134410/new/ https://reviews.llvm.org/D134410 ___ cfe-commits mailing list

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

jmciver updated this revision to Diff 471708. jmciver added a comment. Updating D134410 : [clang][CodeGen] Add noundef metadata to load instructions (preliminary) Add flag -enable-noundef-load-analysis and rebase with main. Refactored noundef metadata

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

jdoerfert added a comment. > However cleanup looks scary. Msan reports maybe 20% of unique tests on our > code base. Many a of them share root cause, but still many unique root causes. On quick looks I see no false report While scary, this also is extremely encouraging to move forward with

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

jmciver added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:1746 + if (auto TyPtr = Ty.getTypePtrOrNull()) { +if (!(TyPtr->isSpecificBuiltinType(BuiltinType::UChar) || vitalybuka wrote: > Taking into account potantial risks from pre-existing

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

vitalybuka added a comment. Comment at: clang/lib/CodeGen/CGExpr.cpp:1746 + if (auto TyPtr = Ty.getTypePtrOrNull()) { +if (!(TyPtr->isSpecificBuiltinType(BuiltinType::UChar) || Taking into account potantial risks from pre-existing UB, I believe we need

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

vitalybuka added a comment. In D134410#3860646 , @xbolva00 wrote: >> I assume with this patch landed, many such cases may change code behavior. >> So we will need to update msan to have a tool to detect cases like this >> anyway. > > I believe that

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

xbolva00 added a comment. > I assume with this patch landed, many such cases may change code behavior. So > we will need to update msan to have a tool to detect cases like this anyway. I believe that updated msan should come first. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

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

vitalybuka added a reviewer: vitalybuka. vitalybuka added a comment. In D134410#3817190 , @vitalybuka wrote: >> Also I applied this patch with D134698 >> and used on our large test set, expecting significant number of

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

jmciver updated this revision to Diff 467690. jmciver edited the summary of this revision. jmciver added a comment. Updating D134410 : [clang][CodeGen] Add noundef metadata to load instructions (preliminary) Resolve merge conflicts due to the adoption of the

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

jmciver added a comment. Decreased CPU loading during test-suite build did lower times in some instances, but not a lot. F24724990: results-subset.txt F24725002: results-full.txt Repository: rG LLVM Github Monorepo

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

jmciver added a comment. In D134410#3816881 , @vitalybuka wrote: > Are these patches uploaded with arc tool? Yes, the patches were uploaded with `arc`. The size of the patch was too large to use the web interface. Repository: rG LLVM Github

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

vitalybuka added a comment. > Also I applied this patch with D134698 and > used on our large test set, expecting significant number of pre-existing > reports. To my surprise I see not many of them. Something wrong with my msan experiment, I'll re-evaluate

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

vitalybuka added a comment. In D134410#3817070 , @xbolva00 wrote: > In D134410#3816918 , @vitalybuka > wrote: > >> I tried to hook this patch into MemorySanitizer and it reduces instrumented >> code by ~30% ! >

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

xbolva00 added a comment. In D134410#3816918 , @vitalybuka wrote: > I tried to hook this patch into MemorySanitizer and it reduces instrumented > code by ~30% ! Wow, amazing results! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

vitalybuka added a comment. I tried to hook this patch into MemorySanitizer and it reduces instrumented code by ~30% ! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134410/new/ https://reviews.llvm.org/D134410

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

vitalybuka added a comment. Are there patches uploaded with arc tool? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134410/new/ https://reviews.llvm.org/D134410 ___ cfe-commits mailing list

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

jmciver added a comment. The following are results from test-suite execution. LHS is a main build (17dde371e773 ) and the RHS is main with patch applied. The `results-subset.txt` is absent of unit, micro, and torture tests.

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

jmciver added a comment. The regression and test-suite pass using a bootstrap build. I'll provide performance data tomorrow. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134410/new/ https://reviews.llvm.org/D134410

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

efriedma added a comment. How much code does this break? How much does this help performance? (My instinct here is that the tradeoff is not worth it, but I'm willing to be convinced otherwise.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

jmciver created this revision. Herald added subscribers: mattd, asavonic, jdoerfert, pengfei. Herald added a project: All. jmciver retitled this revision from "[clang][CodeGen] Add noundef metadata to scalar load instructions" to "[clang][CodeGen] Add noundef metadata to scalar load instructions