[PATCH] D44223: [ms] Emit vtordisp initializers in a deterministic order.

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1207 uint64_t ConstantVBaseOffset = -Layout.getVBaseClassOffset(I->first).getQuantity(); +Layout.getVBaseClass

[PATCH] D44221: Avoid including ScopeInfo.h from Sema.h

rnk updated this revision to Diff 137487. rnk added a comment. - remove Sema::recordEvaluatedWeakUse to fix Linux build https://reviews.llvm.org/D44221 Files: clang/include/clang/Sema/ScopeInfo.h clang/include/clang/Sema/Sema.h clang/include/clang/Sema/SemaLambda.h clang/lib/Sema/Sema.c

[PATCH] D44221: Avoid including ScopeInfo.h from Sema.h

This revision was automatically updated to reflect the committed changes. Closed by commit rL326957: Avoid including ScopeInfo.h from Sema.h (authored by rnk, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D44221?vs=137487&id=137489#to

[PATCH] D44221: Avoid including ScopeInfo.h from Sema.h

This revision was automatically updated to reflect the committed changes. Closed by commit rC326957: Avoid including ScopeInfo.h from Sema.h (authored by rnk, committed by ). Changed prior to commit: https://reviews.llvm.org/D44221?vs=137487&id=137488#toc Repository: rC Clang https://review

[PATCH] D44039: [Sema] Make getCurFunction() return null outside function parsing

This revision was automatically updated to reflect the committed changes. Closed by commit rL326965: [Sema] Make getCurFunction() return null outside function parsing (authored by rnk, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D44

[PATCH] D44039: [Sema] Make getCurFunction() return null outside function parsing

This revision was automatically updated to reflect the committed changes. Closed by commit rC326965: [Sema] Make getCurFunction() return null outside function parsing (authored by rnk, committed by ). Changed prior to commit: https://reviews.llvm.org/D44039?vs=137472&id=137506#toc Repository:

[PATCH] D16632: clang-cl: Take dllexport from original function decl into account

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D16632 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44278: CodeGen: simplify and validate exception personalities

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm, thanks! This is nicer. Repository: rC Clang https://reviews.llvm.org/D44278 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lis

[PATCH] D44482: Set dso_local for NSConcreteStackBlock

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D44482 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44233: Set dso_local on external rtti GVs

rnk accepted this revision. rnk added inline comments. This revision is now accepted and ready to land. Comment at: lib/CodeGen/ItaniumCXXABI.cpp:2618-2620 +const CXXRecordDecl *RD = nullptr; +if (const auto *RecordTy = dyn_cast(Ty)) + RD = cast(RecordTy->getDecl());

[PATCH] D44494: [libunwind] Support __register_frame with a full .eh_frame section

rnk added inline comments. Comment at: src/UnwindCursor.hpp:53 unw_word_t fde, unw_word_t mh)); + static void iterateSections(std::function func); I'm concerned that std::function might introduce runtime dependen

[PATCH] D44493: [libunwind] Add a cmake option for disabling the use of the dynamic linker

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Repository: rUNW libunwind https://reviews.llvm.org/D44493 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bi

[PATCH] D43184: [WIP] [ItaniunCXXABI] Add an option for loading the type info vtable with dllimport

rnk added a comment. In https://reviews.llvm.org/D43184#1031831, @mstorsjo wrote: > For `__cxxabiv1::__class_type_info`, there's no declaration visible in scope > at all, and the actual contents of the vtable of this class (and the other > similar classes) doesn't seem to be defined in the ABI

[PATCH] D44505: [MS] Always use base dtors in place of complete/vbase dtors when possible

rnk created this revision. rnk added reviewers: erichkeane, zahiraam, majnemer, rjmccall. Previously we tried too hard to uphold the fiction that destructor variants work like they do on Itanium throughout the ABI-neutral parts of clang. This lead to MS C++ ABI incompatiblities and other bugs. Now

[PATCH] D43184: [WIP] [ItaniunCXXABI] Add an option for loading the type info vtable with dllimport

rnk added a comment. In https://reviews.llvm.org/D43184#1038396, @mstorsjo wrote: > (Accidentally pressed submit too soon) > > This was my conclusion at some point as well - but there's also a few issues > with that approach which makes me quite hesitant: > > - doing fixups in the code segment r

[PATCH] D43184: [WIP] [ItaniunCXXABI] Add an option for loading the type info vtable with dllimport

rnk added a comment. In https://reviews.llvm.org/D43184#1039354, @mstorsjo wrote: > Ok - I'll put it on the backburner, and maybe see if I'd try implementing the > linker part of fixing unannotated data imports at some point. I'm not sure that's feasible. At least for x86, global addresses can

[PATCH] D44505: [MS] Always use base dtors in place of complete/vbase dtors when possible

This revision was automatically updated to reflect the committed changes. Closed by commit rC327732: [MS] Always use base dtors in place of complete/vbase dtors when possible (authored by rnk, committed by ). Changed prior to commit: https://reviews.llvm.org/D44505?vs=138485&id=138748#toc Repo

[PATCH] D44505: [MS] Always use base dtors in place of complete/vbase dtors when possible

rnk added a comment. Thanks! I'm sorry I wasn't able to express these ideas better in https://reviews.llvm.org/D39063. Repository: rC Clang https://reviews.llvm.org/D44505 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm

[PATCH] D44582: [Builtins] Fix calling long double math functions on x86_64 mingw

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Repository: rC Clang https://reviews.llvm.org/D44582 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mail

[PATCH] D44505: [MS] Always use base dtors in place of complete/vbase dtors when possible

rnk added a comment. And, this appears to have broken clang's self-host: http://lab.llvm.org:8011/builders/clang-x86-windows-msvc2015/builds/10799 Time to debug. Repository: rC Clang https://reviews.llvm.org/D44505 ___ cfe-commits mailing list c

[PATCH] D44505: [MS] Always use base dtors in place of complete/vbase dtors when possible

rnk added a comment. Should be fixed in https://reviews.llvm.org/rL327754. Repository: rC Clang https://reviews.llvm.org/D44505 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44630: [ms] Parse #pragma optimize and ignore it behind its own flag

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm, thanks! Comment at: lib/Parse/ParsePragma.cpp:2970 + + if (Tok.isNot(tok::l_paren)) { +PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_lparen) << "optimize";

[PATCH] D44619: [CodeGen] Add cleanup scope to EmitInlinedInheritingCXXConstructorCall

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Code looks good, but we should make the test more robust and self-documenting. Comment at: test/CodeGenCXX/inheriting-constructor-cleanup.cpp:22 +// CHECK-LABEL: define void @_Z1fv

[PATCH] D44649: Set dso_local for guid decls

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Yep, these are pretty much always local. https://reviews.llvm.org/D44649 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/

[PATCH] D44491: Set dso_local for CFConstantStringClassReference

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D44491 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47480: clang-cl: Expose -no-canonical-prefixes

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Comment at: test/Driver/cl-options.c:595 +// RUN: -no-canonical-prefixes \ +// RUN: -fno-coverage-mapping \ // RUN: --version \ takuto.ikuta wrot

[PATCH] D46664: Fix null MSInheritanceAttr deref in CXXRecordDecl::getMSInheritanceModel()

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. As far as workarounds go, this looks great! Please add a test case, I think you had one in the initial patch. Should I go ahead and commit this? https://reviews.llvm.org/D46664

[PATCH] D47578: Do not enforce absolute path argv0 in windows

rnk added a comment. I think this would be easy to unit test in llvm/unittests/Support/CommandLine.cpp. We'd just check that the filename is "SupportTests.exe" on Windows and the path is relative after calling this, I guess. Right? Look at how ProgramTest.cpp does this to get a reasonable argv0

[PATCH] D46664: Fix null MSInheritanceAttr deref in CXXRecordDecl::getMSInheritanceModel()

This revision was automatically updated to reflect the committed changes. Closed by commit rC333680: Fix null MSInheritanceAttr deref in CXXRecordDecl::getMSInheritanceModel() (authored by rnk, committed by ). Changed prior to commit: https://reviews.llvm.org/D46664?vs=149300&id=149329#toc Rep

[PATCH] D46664: Fix null MSInheritanceAttr deref in CXXRecordDecl::getMSInheritanceModel()

rnk added a comment. Thanks for the patch, I committed it as r333680 with a minor modification. https://reviews.llvm.org/D46664 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47564: [Parse] Use CapturedStmt for @finally on MSVC

rnk added a comment. Using CapturedStmt to do frontend outlining was the direction I suggested. I want to hear what @rsmith and @rjmccall think, though. Comment at: lib/Parse/ParseObjc.cpp:2588 + bool ShouldCapture = Actions.getASTContext() +

[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning

rnk added a comment. I'm in favor of this direction. The `var || var && truth_constant` pattern match seems much more robust than the macro pattern match. It's also consistent with what we do outside of macros, so it's less special and surprising. https://reviews.llvm.org/D47687 ___

[PATCH] D47672: [Headers] Add _Interlocked*_HLEAcquire/_HLERelease

rnk added subscribers: craig.topper, chandlerc. rnk added a comment. I read up a little bit on TSX and HLE: https://software.intel.com/en-us/node/524022 https://en.wikipedia.org/wiki/Transactional_Synchronization_Extensions These HLE variants of the usual atomic exchange intrinsics add the `xacqu

[PATCH] D46042: Cap vector alignment at 16 for all Darwin platforms

rnk added a comment. This change appears to have caused some blink vector math unit tests to fail on Windows. We are tracking it at https://crbug.com/849251. It has a pretty small reproducer: #include __m256 loadit(__m256 *p) { return _mm256_loadu_ps((const float *)p); } Compile for x86_6

[PATCH] D46042: Cap vector alignment at 16 for all Darwin platforms

rnk added a comment. It's the typedef alignment changes that are causing problems for us, not the MaxVectorAlign changes. That makes more sense. The new alignment attribute breaks our implementation of `_mm256_loadu_ps`, because the packed struct ends up with a 32-byte alignment. Here's the imp

[PATCH] D46042: Cap vector alignment at 16 for all Darwin platforms

rnk added a comment. In https://reviews.llvm.org/D46042#1121674, @rjmccall wrote: > > I think we should revert this for now. Adding the alignment attribute to > > all Intel vector typedefs is a bigger change than it seems. > > Ugh. That is just an awful language rule. Would it be reasonable t

[PATCH] D46042: Cap vector alignment at 16 for all Darwin platforms

rnk added a comment. By the way, I went ahead and reverted this in r333958. Repository: rC Clang https://reviews.llvm.org/D46042 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47784: [MS][ARM64]: Promote _setjmp to_setjmpex as there is no _setjmp in the ARM64 libvcruntime.lib

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm I'm happy to refactor this after landing it as is, or you can do it if you like. Comment at: lib/CodeGen/CGBuiltin.cpp:2808 case Builtin::BI_setjmpex: { if (getTarget

[PATCH] D47672: [Headers] Add _Interlocked*_HLEAcquire/_HLERelease

rnk added a comment. In https://reviews.llvm.org/D47672#1122013, @ethanhs wrote: > In https://reviews.llvm.org/D47672#1121181, @rnk wrote: > > > > > > > > > They are a hint to the processor that this is a short critical section, and > > it is likely that the entire critical section can be entere

[PATCH] D47784: [MS][ARM64]: Promote _setjmp to_setjmpex as there is no _setjmp in the ARM64 libvcruntime.lib

This revision was automatically updated to reflect the committed changes. Closed by commit rC334112: [MS][ARM64]: Promote _setjmp to_setjmpex as there is no _setjmp in the ARM64… (authored by rnk, committed by ). Changed prior to commit: https://reviews.llvm.org/D47784?vs=149984&id=150171#toc

[PATCH] D47784: [MS][ARM64]: Promote _setjmp to_setjmpex as there is no _setjmp in the ARM64 libvcruntime.lib

rnk added a comment. Thanks for the patch! Repository: rC Clang https://reviews.llvm.org/D47784 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47850: [Driver] Stop passing -fseh-exceptions for x86_64-windows-msvc

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Repository: rC Clang https://reviews.llvm.org/D47850 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mail

[PATCH] D47853: [Frontend] Disallow non-MSVC exception models for windows-msvc targets

rnk accepted this revision. rnk added a comment. lgtm Repository: rC Clang https://reviews.llvm.org/D47853 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47862: [CodeGen] Always use MSVC personality for windows-msvc targets

rnk added inline comments. Comment at: lib/CodeGen/CGException.cpp:134-135 const llvm::Triple &T = Target.getTriple(); + if (T.isWindowsMSVCEnvironment()) +return EHPersonality::MSVC_CxxFrameHandler3; + I guess previously we carefully arranged to go to wh

[PATCH] D47687: fix: [Bug 18971] - Missing -Wparentheses warning

rnk added subscribers: dexonsmith, chandlerc. rnk added a comment. @dexonsmith is there someone from Apple who can comment on rdar://8678458 and the merits of disabling this warning in macros? I strongly suspect the original report was dealing with code like `assert(x || y && "str");`, if so we

[PATCH] D47862: [CodeGen] Always use MSVC personality for windows-msvc targets

rnk accepted this revision. rnk added inline comments. This revision is now accepted and ready to land. Comment at: lib/CodeGen/CGException.cpp:134-135 const llvm::Triple &T = Target.getTriple(); + if (T.isWindowsMSVCEnvironment()) +return EHPersonality::MSVC_CxxFrameHand

[PATCH] D47956: [MS] Consder constexpr globals to be inline, as in C++17

rnk created this revision. rnk added a reviewer: thakis. Microsoft seems to do this regardless of the language mode, so we must also do it in order to be ABI compatible. Fixes PR36125 https://reviews.llvm.org/D47956 Files: clang/lib/Sema/SemaDecl.cpp clang/test/CXX/dcl.dcl/dcl.spec/dcl.con

[PATCH] D47672: [Headers] Add _Interlocked*_HLEAcquire/_HLERelease

rnk added a comment. In https://reviews.llvm.org/D47672#1128308, @hans wrote: > It sounds like adding proper support for HLE prefixes is a largeish project. > > ctopper, rnk: Do you think it would be worth adding inline asm versions (with > the xacquire/release prefixes) of these intrinsics in t

[PATCH] D47988: [CodeGen] Emit MSVC funclet IR for Obj-C exceptions

rnk added a comment. In https://reviews.llvm.org/D47988#1127432, @smeenai wrote: > Any idea why we would see inlining in one case and not the other? i686 vs. > x86-64 doesn't make any difference, and neither does -Os vs. -O1 vs. -O2. My theory is that the inliner is attempting to avoid inlinin

[PATCH] D38680: [libunwind] Fix handling of DW_CFA_GNU_args_size

rnk added a comment. In https://reviews.llvm.org/D38680#1123018, @joerg wrote: > After a careful review of newer GCC / libgcc and the assembler annotations > from LLVM, I have come to the following conclusions: > > (1) The semantics have been somewhat changed by GCC in recent years. There is >

[PATCH] D48224: Don't let test/Driver/no-canonical-prefixes.c form a symlink cycle the second time it runs.

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D48224 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48296: clang-cl: Emit normal narrowing diagnostics for initializer lists if -fmsc-version is at least 1900 (i.e. MSVC2015).

rnk added a comment. Do you think we should go ahead and remove the `DefaultIgnore` on these warnings as well? At this point, approximately nobody will see them. Comment at: clang/lib/Sema/SemaInit.cpp:8367-8370 +bool DiagErr = +S.getLangOpts().CPlusPlus11 && +

[PATCH] D47988: [CodeGen] Emit MSVC funclet IR for Obj-C exceptions

rnk added a comment. In https://reviews.llvm.org/D47988#1135533, @rjmccall wrote: > In general, it's unfortunate that this has to leave so many > C++-runtime-specific tendrils in the ObjC code. Unlike the EH type patch, > though, I'm not sure I can see a great alternative here, especially beca

[PATCH] D48296: clang-cl: Emit normal narrowing diagnostics for initializer lists if -fmsc-version is at least 1900 (i.e. MSVC2015).

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D48296#1136095, @thakis wrote: > Thanks! I'd keep it DefaultIgnored: I'd be annoyed if I'd get these by > default in C++98 mode. Got it, for some reason I'd forgotten t

[PATCH] D46652: [clang-cl, PCH] Implement support for MS-style PCH through headers

rnk added a comment. @hans, think you'll have time to look at this with your recent dllexport PCH experimentation? https://reviews.llvm.org/D46652 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinf

[PATCH] D47956: [MS] Consder constexpr globals to be inline, as in C++17

rnk added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:6597 + (getLangOpts().CPlusPlus17 || + Context.getTargetInfo().getCXXABI().isMicrosoft())) NewVD->setImplicitlyInline(); thakis wrote: > Is this related to /Zc:externCo

[PATCH] D47956: [MS] Consder constexpr globals to be inline, as in C++17

rnk added a comment. In https://reviews.llvm.org/D47956#1138521, @rsmith wrote: > Can we now remove the corresponding MSVC-specific hacks elsewhere (eg, > `ASTContext::isMSStaticDataMemberInlineDefinition`), or do we still need > those for `const`-but-not-`constexpr` static data members? We s

[PATCH] D48187: [Intrinsics] Add/move some builtin declarations in intrin.h to get ms-intrinsics.c to not issue warnings

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D48187 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48448: [X86] Correct the inline assembly implementations of __movsb/w/d/q and __stosw/d/q to mark registers/memory as modified

rnk added a comment. Yikes. Repository: rL LLVM https://reviews.llvm.org/D48448 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48426: [clang-cl] Don't emit dllexport inline functions etc. from pch files (PR37801)

rnk added a subscriber: dblaikie. rnk added a comment. `LangOpts.ModulesCodegen` is very related in spirit to this, but I think we need a distinct option because that was designed to handle all inline functions (too much), not just dllexport inline functions. + @dblaikie Comm

[PATCH] D52990: [MinGW] Fix passing a sanitizer lib name as dependent lib

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D52990 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52193: RFC: [clang] Multithreaded compilation support

rnk added a comment. In https://reviews.llvm.org/D52193#1260933, @aganea wrote: > I wrote a fully fledged crash reporting system which does that, so I know > that's possible. The tricky thing is to ensure > `Driver::generateCompilationDiagnostics()` doesn't touch potentially invalid > structur

[PATCH] D52773: clang-cl: Add /showFilenames option (PR31957)

rnk accepted this revision. rnk added a comment. lgtm Comment at: lib/Driver/Job.cpp:319 + if (PrintInputFilenames) { +for (const char *Arg : InputFilenames) + llvm::outs() << llvm::sys::path::filename(Arg) << "\n"; Huh, a -cc1 action can have multipl

[PATCH] D53052: [AST] Use -fvisibility value when ignoring -fv-i-h* inline static locals

This revision was automatically updated to reflect the committed changes. Closed by commit rC344190: [AST] Use -fvisibility value when ignoring -fv-i-h* inline static locals (authored by rnk, committed by ). Herald added a subscriber: cfe-commits. Changed prior to commit: https://reviews.llvm.o

[PATCH] D53151: [python] [tests] Fix calling tests on Windows (hopefully)

rnk added a comment. lgtm The cmake builtin tools are pretty useful. =) Repository: rC Clang https://reviews.llvm.org/D53151 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53195: [MinGW] Allow using LTO when lld is used as linker

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Comment at: lib/Driver/ToolChains/MinGW.cpp:383 + Args.getLastArgValue(options::OPT_fuse_ld_EQ, CLANG_DEFAULT_LINKER) + .equals_lower("lld"); }

[PATCH] D53066: [RFC] [MinGW] Print paths with forward slashes in commands printed with -v

rnk added inline comments. Comment at: lib/Driver/Job.cpp:104-105 + std::string Buf; + if (PathStyle == llvm::sys::path::Style::posix) { +Buf = llvm::sys::path::convert_to_slash(Arg); +Arg = Buf; This is blindly treating every argument as if it were a p

[PATCH] D53066: [RFC] [MinGW] Print paths with forward slashes in commands printed with -v

rnk added inline comments. Comment at: lib/Driver/Job.cpp:104-105 + std::string Buf; + if (PathStyle == llvm::sys::path::Style::posix) { +Buf = llvm::sys::path::convert_to_slash(Arg); +Arg = Buf; mstorsjo wrote: > rnk wrote: > > This is blindly treating

[PATCH] D53210: Revert 344389 "Revert r344375 "[Driver] check for exit code from SIGPIPE""

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. I think it's fine, just remove the git hash from the commit message and just refer to the svn number. Repository: rC Clang https://reviews.llvm.org/D53210

[PATCH] D53135: Remove top-level using declaration from header files, as these aliases leak.

rnk added inline comments. Comment at: cfe/trunk/include/clang/Serialization/GlobalModuleIndex.h:147 static std::pair - readIndex(StringRef Path); + readIndex(llvm::StringRef Path); It's preferred to include clang/Basic/LLVM.h instead and use the StringRef

[PATCH] D53497: [AST] Do not align virtual bases in `MicrosoftRecordLayoutBuilder` when an external layout is used

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. > Also it seems that MicrosoftRecordLayoutBuilder with an external source and > without one differ considerably, may be it is worth to split this and to > create two different builders? I think that

[PATCH] D53457: clang-cl: Add "/Xdriver:" pass-through arg support.

rnk added a comment. In https://reviews.llvm.org/D53457#1269769, @hans wrote: > I haven't started looking at the code yet. > > I'm not completely convinced that we want this. So far we've used the > strategy of promoting clang options that are also useful in clang-cl to core > options, and if s

[PATCH] D53476: [clang-cl] Allowed -fopenmp work and link openmp library from per-runtime library directory

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Repository: rC Clang https://reviews.llvm.org/D53476 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mail

[PATCH] D53476: [clang-cl] Allowed -fopenmp work and link openmp library from per-runtime library directory

rnk requested changes to this revision. rnk added a comment. This revision now requires changes to proceed. Actually, can you please add -fopenmp to `test/Driver/cl-options.c` to verify that we accept `-fopenmp`? Repository: rC Clang https://reviews.llvm.org/D53476 ___

[PATCH] D53066: [RFC] [Driver] Use forward slashes in most linker arguments

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm https://reviews.llvm.org/D53066 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52441: [CodeGen] Update min-legal-vector width based on function argument and return types

rnk added a comment. In https://reviews.llvm.org/D52441#1258317, @craig.topper wrote: > Address Reid's comments. Add a comment with a list of all things that > currently effect the vector width attribute emitted in IR. > > For inlining, we update the caller's attribute during merging to ensure i

[PATCH] D53066: [Driver] Use forward slashes in most linker arguments

rnk added inline comments. Comment at: lib/Driver/Driver.cpp:1013-1014 } + for (auto *Str : {&Dir, &InstalledDir, &SysRoot, &DyldPrefix, &ResourceDir}) +*Str = llvm::sys::path::convert_to_slash(*Str); zturner wrote: > Is this going to affect the cl driv

[PATCH] D53618: [AArch64] [Windows] Emit unwind tables by default.

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Yep, land it after the dependency. Comment at: lib/Driver/ToolChains/MSVC.cpp:725-727 // Emit unwind tables by default on Win64. All non-x86_32 Windows platforms // such as A

[PATCH] D53586: Implement Function Multiversioning for Non-ELF Systems.

rnk added a comment. Seems reasonable. Should the resolver still be called `?foo@.resolver`, or should it get a new name, since it is quite functionally different now? Comment at: lib/CodeGen/CodeGenFunction.cpp:2381 + +template +static void CreateMultiVersionResolverReturn(ll

[PATCH] D53441: [ms] Prevent explicit constructor name lookup if scope is missing

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm, thanks! Repository: rC Clang https://reviews.llvm.org/D53441 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi

[PATCH] D53586: Implement Function Multiversioning for Non-ELF Systems.

rnk added inline comments. Comment at: lib/CodeGen/CodeGenFunction.cpp:2391-2393 + llvm::SmallVector Args; + llvm::for_each(Resolver->args(), + [&](llvm::Argument &Arg) { Args.push_back(&Arg); }); erichkeane wrote: > rnk wrote: > > Surely this w

[PATCH] D53586: Implement Function Multiversioning for Non-ELF Systems.

rnk added inline comments. Comment at: lib/CodeGen/CodeGenFunction.cpp:2395 + + llvm::CallInst *Result = Builder.CreateCall(FuncToReturn, Args); + rnk wrote: > erichkeane wrote: > > erichkeane wrote: > > > rnk wrote: > > > > This approach is... not going to work

[PATCH] D40925: Add option -fkeep-static-consts

rnk added a subscriber: zturner. rnk added a comment. My coworker, @zturner, asked if I knew a way to force emit all constants, and I mentioned this flag, but we noticed it doesn't work on cases when the constant is implicitly static, like this: const int foo = 42; If I add `static` to it, i

[PATCH] D53586: Implement Function Multiversioning for Non-ELF Systems.

rnk accepted this revision. rnk added a comment. Here's a thought. What happens if different TUs observe different overload sets, perhaps because of ifdefs? Different TUs will generate different resolvers, but they won't dispatch to the same sets of targets. I'm guessing we'd treat that as an O

[PATCH] D53066: [Driver] Use forward slashes in most linker arguments

rnk accepted this revision. rnk added a comment. lgtm https://reviews.llvm.org/D53066 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53787: [Sema] Use proper visibility for global new and delete declarations

rnk added a comment. This seems pretty inconsistent with how -fvisibility=hidden behaves with normal, user written declarations. Consider this code: void foo(); void bar() { foo(); } We get this IR: $ clang -S -emit-llvm --target=x86_64-linux t.c -o - -fvisibility=hidden ... define h

[PATCH] D53870: [clang-cl] Put dllexport attrs on static locals also in template instantiations (PR39496)

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Comment at: test/CodeGenCXX/dllimport.cpp:1010 +int bar() { T t; return t.foo(); } +// MO1-DAG: @"?x@?{{1|2}}??foo@?$T@H@pr39496@@Q{{[A-Z]*}}HXZ@4HA" = available_externally d

[PATCH] D53066: [Driver] Use forward slashes in most linker arguments

rnk added a comment. We discussed adding a new path style that opts into a / preferred separator on Windows, something like path::Style::windows_forward. All of the absolute path handling routines would behave the same, but the preferred separator would now be `/`. We can make the default path

[PATCH] D53912: [Headers] [MS] Add intrin0.h

rnk added a comment. This sounds like it would defeat what I'm assuming is the intended purpose of intrin0.h, which is to reduce compile time. intrin.h is kind of enormous, and the compile time problems are well-documented. We should investigate what's up with intrin0.h and implement as many bu

[PATCH] D53916: [ARM64] [Windows] Implement _InterlockedExchangeAdd*_* builtins.

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Comment at: lib/CodeGen/CGBuiltin.cpp:100 +CodeGenFunction &CGF, llvm::AtomicRMWInst::BinOp Kind, const CallExpr *E, +AtomicOrdering Ordering = AtomicOrdering::Sequent

[PATCH] D56671: [COFF, ARM64] Add __nop intrinsic

rnk accepted this revision. rnk added a comment. lgtm Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56671/new/ https://reviews.llvm.org/D56671 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm

[PATCH] D56463: [SEH] Pass the frame pointer from SEH finally to finally functions

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Thanks! I'm surprised we passed as much of the Microsoft exception tests as we did with this bug. Maybe it regressed. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D53541: [COFF, ARM64] Do not emit x86_seh_recoverfp intrinsic

rnk added a comment. In D53541#1353251 , @mgrang wrote: > > What about the three stack pointer case of stack realignment plus a dynamic > > alloca? Typically this is the case where recoverfp is necessary. > > @rnk Sorry, I missed your comment earlier. I a

[PATCH] D56686: [X86] Make _xgetbv/_xsetbv on non-windows platforms

rnk added inline comments. Comment at: lib/Headers/immintrin.h:381-394 +#ifdef _MSC_VER +#ifdef __cplusplus +extern "C" { +#endif +unsigned __int64 __cdecl _xgetbv(unsigned int); +void __cdecl _xsetbv(unsigned int, unsigned __int64); +#ifdef __cplusplus I think w

[PATCH] D56686: [X86] Make _xgetbv/_xsetbv on non-windows platforms

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56686/new/ https://reviews.llvm.org/D56686 ___ cfe-commits mailing list cfe-commits@lists.l

[PATCH] D55781: Make CC mangling conditional on the ABI version

rnk abandoned this revision. rnk added a comment. In D55781#1351541 , @rjmccall wrote: > Okay. In my opinion, then, we should just do this unconditionally. Sounds good to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55781/new/ https://re

[PATCH] D53541: [COFF, ARM64] Do not emit x86_seh_recoverfp intrinsic

rnk added a comment. I can't compile the example you gave yet because I haven't applied your patches locally, but this is the "three stack pointer" case that I have in mind: struct Foo { void (*ptr)(); int x, y, z; }; void escape(void *); void large_align(int a0, int a1, int a

[PATCH] D53541: [COFF, ARM64] Do not emit x86_seh_recoverfp intrinsic

rnk added a comment. In D53541#1358310 , @mgrang wrote: > @rnk Thanks a lot for the clarification. Yes, I see o.x: 0 instead of 42. > Supporting this case would mean implementing recoverfp as well as support > generating the correct parent frame offset f

[PATCH] D56852: [AArch64] Use LLU for 64-bit crc32 arguments

rnk added inline comments. Comment at: include/clang/Basic/BuiltinsAArch64.def:54 +BUILTIN(__builtin_arm_crc32d, "UiUiLLUi", "nc") +BUILTIN(__builtin_arm_crc32cd, "UiUiLLUi", "nc") If we can't change the signature on Linux (I don't see any reason why couldn't,

[PATCH] D56905: [libunwind] [SjLj] Don't use __declspec(thread) in MinGW mode

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Repository: rUNW libunwind CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56905/new/ https://reviews.llvm.org/D56905 ___ cfe-commits ma

<    1   2   3   4   5   6   7   8   9   10   >