[PATCH] [OPENMP] A helper for marking intstructions with llvm.mem.parallel_loop_access
Michael Wong
fraggamuffin at gmail.com
Tue May 20 00:49:27 PDT 2014
Sorry this is in digest format(but I am changing it). But for the fix
above.
Code looks good. One minor question and I suspect this is because you are
preparing the following function for other things in future. As it stands,
it does not use any of the other parameters but the first one in the call
to LoopStack.
Is that the reason this function have excessive unused parameters?
void CodeGenFunction::InsertHelper(llvm::Instruction *I,
const llvm::Twine &Name,
llvm::BasicBlock *BB,
llvm::BasicBlock::iterator InsertPt) const {
LoopStack.InsertHelper(I);
}
Otherwise, LGTM and builds fine.
On Wed, May 7, 2014 at 4:56 PM, <cfe-commits-request at cs.uiuc.edu> wrote:
> Send cfe-commits mailing list submissions to
> cfe-commits at cs.uiuc.edu
>
> To subscribe or unsubscribe via the World Wide Web, visit
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> or, via email, send a message with subject or body 'help' to
> cfe-commits-request at cs.uiuc.edu
>
> You can reach the person managing the list at
> cfe-commits-owner at cs.uiuc.edu
>
> When replying, please edit your Subject line so it is more specific
> than "Re: Contents of cfe-commits digest..."
>
>
> Today's Topics:
>
> 1. Re: PATCH for libcxxabi - harmonize nmstr implementations
> (Joerg Sonnenberger)
> 2. [libcxxabi] r208246 - Make libc++abi use the implementation
> of __numstr from libc++. No functionality change, just
> removal of
> duplicated code. (Marshall Clow)
> 3. Re: [PATCH] [OPENMP] A helper for marking intstructions with
> llvm.mem.parallel_loop_access (Alexander Musman)
> 4. Re: [PATCH] Suggest fix-it ':' when '=' used in
> for-range-declaration while initializing an auto (Ismail
> Pazarbasi)
>
>
> ----------------------------------------------------------------------
>
> Message: 1
> Date: Wed, 7 May 2014 22:11:36 +0200
> From: Joerg Sonnenberger <joerg at britannica.bec.de>
> To: cfe-commits at cs.uiuc.edu
> Subject: Re: PATCH for libcxxabi - harmonize nmstr implementations
> Message-ID: <20140507201136.GA18859 at britannica.bec.de>
> Content-Type: text/plain; charset=utf-8
>
> On Sun, May 04, 2014 at 12:37:04PM -0700, Marshall Clow wrote:
> > Now that jboerg has made the __nmstr implementation in libc++ match the
> one in libc++abi,
> > it?s time to kill one off one of the two implementations.
> >
> > Make libc++abi include the version from libc++.
>
> LGTM.
>
> Joerg
>
>
> ------------------------------
>
> Message: 2
> Date: Wed, 07 May 2014 20:17:41 -0000
> From: Marshall Clow <mclow.lists at gmail.com>
> To: cfe-commits at cs.uiuc.edu
> Subject: [libcxxabi] r208246 - Make libc++abi use the implementation
> of __numstr from libc++. No functionality change, just
> removal of
> duplicated code.
> Message-ID: <20140507201741.45CD02A6C03C at llvm.org>
> Content-Type: text/plain; charset="utf-8"
>
> Author: marshall
> Date: Wed May 7 15:17:41 2014
> New Revision: 208246
>
> URL: http://llvm.org/viewvc/llvm-project?rev=208246&view=rev
> Log:
> Make libc++abi use the implementation of __numstr from libc++. No
> functionality change, just removal of duplicated code.
>
> Modified:
> libcxxabi/trunk/src/stdexcept.cpp
>
> Modified: libcxxabi/trunk/src/stdexcept.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/libcxxabi/trunk/src/stdexcept.cpp?rev=208246&r1=208245&r2=208246&view=diff
>
> ==============================================================================
> --- libcxxabi/trunk/src/stdexcept.cpp (original)
> +++ libcxxabi/trunk/src/stdexcept.cpp Wed May 7 15:17:41 2014
> @@ -7,6 +7,7 @@
> //
>
> //===----------------------------------------------------------------------===//
>
> +#include "__refstring"
> #include "stdexcept"
> #include "new"
> #include <cstdlib>
> @@ -14,147 +15,25 @@
> #include <cstdint>
> #include <cstddef>
>
> -#if __APPLE__
> -#include <dlfcn.h>
> -#include <mach-o/dyld.h>
> -#endif
> -
> -// Note: optimize for size
> -
> -#pragma GCC visibility push(hidden)
> -
> -namespace
> -{
> -
> -class __libcpp_nmstr
> -{
> -private:
> - const char* str_;
> -
> - typedef int count_t;
> -
> - struct _Rep_base
> - {
> - std::size_t len;
> - std::size_t cap;
> - count_t count;
> - };
> -
> - static const std::ptrdiff_t offset =
> static_cast<std::ptrdiff_t>(sizeof(_Rep_base));
> -
> - count_t& count() const _NOEXCEPT {return ((_Rep_base*)(str_ -
> offset))->count;}
> -
> -#if __APPLE__
> - static
> - const void*
> - compute_gcc_empty_string_storage() _NOEXCEPT
> - {
> - void* handle = dlopen("/usr/lib/libstdc++.6.dylib", RTLD_NOLOAD);
> - if (handle == 0)
> - return 0;
> - return (const char*)dlsym(handle,
> "_ZNSs4_Rep20_S_empty_rep_storageE") + offset;
> - }
> -
> - static
> - const void*
> - get_gcc_empty_string_storage() _NOEXCEPT
> - {
> - static const void* p = compute_gcc_empty_string_storage();
> - return p;
> - }
> -#endif
> -
> -public:
> - explicit __libcpp_nmstr(const char* msg);
> - __libcpp_nmstr(const __libcpp_nmstr& s) _NOEXCEPT;
> - __libcpp_nmstr& operator=(const __libcpp_nmstr& s) _NOEXCEPT;
> - ~__libcpp_nmstr();
> - const char* c_str() const _NOEXCEPT {return str_;}
> -};
> -
> -__libcpp_nmstr::__libcpp_nmstr(const char* msg)
> -{
> - std::size_t len = strlen(msg);
> - str_ = static_cast<const char*>(::operator new(len + 1 + offset));
> - _Rep_base* c = (_Rep_base*)str_;
> - c->len = c->cap = len;
> - str_ += offset;
> - count() = 0;
> - std::memcpy(const_cast<char*>(c_str()), msg, len + 1);
> -}
> -
> -inline
> -__libcpp_nmstr::__libcpp_nmstr(const __libcpp_nmstr& s) _NOEXCEPT
> - : str_(s.str_)
> -{
> -#if __APPLE__
> - if (str_ != get_gcc_empty_string_storage())
> -#endif
> - __sync_add_and_fetch(&count(), 1);
> -}
> -
> -__libcpp_nmstr&
> -__libcpp_nmstr::operator=(const __libcpp_nmstr& s) _NOEXCEPT
> -{
> - const char* p = str_;
> - str_ = s.str_;
> -#if __APPLE__
> - if (str_ != get_gcc_empty_string_storage())
> -#endif
> - __sync_add_and_fetch(&count(), 1);
> -#if __APPLE__
> - if (p != get_gcc_empty_string_storage())
> -#endif
> - if (__sync_add_and_fetch((count_t*)(p-sizeof(count_t)),
> count_t(-1)) < 0)
> - {
> - ::operator delete(const_cast<char*>(p-offset));
> - }
> - return *this;
> -}
> -
> -inline
> -__libcpp_nmstr::~__libcpp_nmstr()
> -{
> -#if __APPLE__
> - if (str_ != get_gcc_empty_string_storage())
> -#endif
> - if (__sync_add_and_fetch(&count(), count_t(-1)) < 0)
> - {
> - ::operator delete(const_cast<char*>(str_ - offset));
> - }
> -}
> -
> -}
> -
> -#pragma GCC visibility pop
> +static_assert(sizeof(std::__libcpp_refstring) == sizeof(const char *),
> "");
>
> namespace std // purposefully not using versioning namespace
> {
>
> -logic_error::~logic_error() _NOEXCEPT
> -{
> - __libcpp_nmstr& s = (__libcpp_nmstr&)__imp_;
> - s.~__libcpp_nmstr();
> -}
> +logic_error::~logic_error() _NOEXCEPT {}
>
> const char*
> logic_error::what() const _NOEXCEPT
> {
> - __libcpp_nmstr& s = (__libcpp_nmstr&)__imp_;
> - return s.c_str();
> + return __imp_.c_str();
> }
>
> -runtime_error::~runtime_error() _NOEXCEPT
> -{
> - __libcpp_nmstr& s = (__libcpp_nmstr&)__imp_;
> - s.~__libcpp_nmstr();
> -}
> +runtime_error::~runtime_error() _NOEXCEPT {}
>
> const char*
> runtime_error::what() const _NOEXCEPT
> {
> - __libcpp_nmstr& s = (__libcpp_nmstr&)__imp_;
> - return s.c_str();
> + return __imp_.c_str();
> }
>
> domain_error::~domain_error() _NOEXCEPT {}
>
>
>
>
> ------------------------------
>
> Message: 3
> Date: Thu, 8 May 2014 00:45:21 +0400
> From: Alexander Musman <alexander.musman at gmail.com>
> To: Hal Finkel <hfinkel at anl.gov>
> Cc: Alexey Bataev <a.bataev at hotmail.com>, rjmccall at gmail.com, C
> Bergstr?m <cbergstrom at pathscale.com>, Richard Smith
> <richard at metafoo.co.uk>, llvm cfe <cfe-commits at cs.uiuc.edu>
> Subject: Re: [PATCH] [OPENMP] A helper for marking intstructions with
> llvm.mem.parallel_loop_access
> Message-ID:
> <CAPMY8no+Z1TQDMxQ6HSrhTtbOu7g2wM=-
> d+epmfFNVdPv3YMBg at mail.gmail.com>
> Content-Type: text/plain; charset="utf-8"
>
> Hi Tyler, Hal,
>
> > Looks like this implementation is an alternative to the patch I am
> proposing.
>
> I agree with Hal, I think both approaches (#pragma loop vectorize and
> #pragma omp simd) can co-exist.
> #pragma omp simd follows Openmp standard and #pragma loop vectorize has
> more freedom to add any hints specific for tuning vectorizer.
>
> 1) Actually it is part of AST - for example, see
> test/OpenMP/simd_ast_print.cpp for serialization and ast printing. There is
> a separate AST node (see OMPSimdDirective declaration in
> include/clang/AST/StmtOpenMP.h).
>
> 2) Templates - this part is also implemented, there is a test for non-type
> parameter in 'safelen' clause in the same file. If you are interested how
> template instatiation is done, I suggest to look at
> TransformOMPSafelenClause (in lib/Sema/TreeTransform.h). (I'm not sure that
> the same approach will work for pragma loop vectorize though, because it
> uses attributes while OMPSimdDirective has a separate AST node).
>
> 3) No, it is not a separate pass - routine InsertHelper is called for each
> instruction from IR Builder, so that, when it is in the "parallel" loop
> body, it will mark load/store instructions with
> llvm.mem.parallel_loop_access.
>
> Thanks,
> Alexander
>
>
> 2014-05-07 22:47 GMT+04:00 Hal Finkel <hfinkel at anl.gov>:
>
> >
> >
> > ----- Original Message -----
> > > From: "Tyler Nowicki" <tnowicki at apple.com>
> > > To: "Alexander Musman" <alexander.musman at gmail.com>
> > > Cc: "Douglas Gregor" <dgregor at apple.com>, rjmccall at gmail.com, "Richard
> > Smith" <richard at metafoo.co.uk>, "Alexey
> > > Bataev" <a.bataev at hotmail.com>, "Hal Finkel" <hfinkel at anl.gov>,
> > gribozavr at gmail.com, cbergstrom at pathscale.com,
> > > "renato golin" <renato.golin at linaro.org>, "llvm cfe" <
> > cfe-commits at cs.uiuc.edu>
> > > Sent: Wednesday, May 7, 2014 12:18:17 PM
> > > Subject: Re: [PATCH] [OPENMP] A helper for marking intstructions with
> > llvm.mem.parallel_loop_access
> > >
> > > Hi Alexander,
> > >
> > > Looks like this implementation is an alternative to the patch I am
> > > proposing.
> >
> > Just to provide a small amount of additional context:
> >
> > OpenMP 4 specifies standard pragmas for loop vectorization. These differ,
> > generically, from vendor-specific vectorization pragmas because they 1)
> > assert safety and must work (or produce an error; they are not just
> hints)
> > and 2) the standard specifies a restricted syntax for the loops which the
> > frontend must check. This is not an alternative to what you've proposed:
> we
> > need both! That having been said, we should obviously unify the
> > implementation to the extent possible.
> >
> > -Hal
> >
> > > I?m pretty sure I also saw something like this on the
> > > list before. There are a few things I am concerned about with this
> > > approach. I am new to clang development though so please let me know
> > > what I misunderstood anything.
> > >
> > > 1. The pragma is not part of the AST. This makes the pragmas hidden
> > > to any passes over the AST. That may cause problems for features
> > > like serialization/deserialization and ast-print. If the pragma is
> > > an AST node those are easily supported.
> > >
> > > 2. Templates. I know there is a lot of interest in allowing the loop
> > > vectorization pragmas to take non-type template parameters. It has
> > > been a much requested feature on my patch. One of the challenges I
> > > am finding is that the non-type template parameters should be
> > > resolved on template instantiation. This probably has to occur when
> > > AST nodes are visited during semantic analysis. A few people have
> > > commented that they don?t want it in CodeGen because that would mean
> > > the verification of the input values would occur in codegen as well
> > > (see link [1] below). I?m still trying to figure this one out. But I
> > > am wondering how would you do it?
> > >
> > > 2. Efficiency. It looks like you have to make an extra pass over
> > > every loop.cond and loop.body even if metadata was not attached to
> > > those loops? Or did I miss something?
> > >
> > > As I said, I?m new to clang so if I misunderstood anything please let
> > > me know.
> > >
> > > Tyler
> > >
> > > [1] -
> > >
> >
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20140505/104925.html
> > >
> > > On May 7, 2014, at 6:13 AM, Alexander Musman
> > > <alexander.musman at gmail.com> wrote:
> > >
> > > > Hi doug.gregor, rjmccall, rsmith, ABataev, hfinkel, gribozavr,
> > > > cbergstrom, rengolin, TylerNowicki,
> > > >
> > > > This patch adds a helper class (CGLoopInfo) for marking memory
> > > > instructions with llvm.mem.parallel_loop_access metadata.
> > > > It also adds a simple initial version of codegen for pragma omp
> > > > simd (it will change in the future to support all the clauses).
> > > >
> > > > http://reviews.llvm.org/D3644
> > > >
> > > > Files:
> > > > lib/CodeGen/CGBuilder.h
> > > > lib/CodeGen/CGLoopInfo.cpp
> > > > lib/CodeGen/CGLoopInfo.h
> > > > lib/CodeGen/CGStmt.cpp
> > > > lib/CodeGen/CGStmtOpenMP.cpp
> > > > lib/CodeGen/CMakeLists.txt
> > > > lib/CodeGen/CodeGenFunction.cpp
> > > > lib/CodeGen/CodeGenFunction.h
> > > > test/OpenMP/simd_metadata.c
> > > > <D3644.9161.patch>
> > >
> > >
> >
> > --
> > Hal Finkel
> > Assistant Computational Scientist
> > Leadership Computing Facility
> > Argonne National Laboratory
> >
> -------------- next part --------------
> An HTML attachment was scrubbed...
> URL: <
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140508/c627e2ee/attachment-0001.html
> >
>
> ------------------------------
>
> Message: 4
> Date: Wed, 7 May 2014 20:56:46 +0000
> From: Ismail Pazarbasi <ismail.pazarbasi at gmail.com>
> To: ismail.pazarbasi at gmail.com, benny.kra at gmail.com,
> dblaikie at gmail.com
> Cc: richard at metafoo.co.uk, cfe-commits at cs.uiuc.edu
> Subject: Re: [PATCH] Suggest fix-it ':' when '=' used in
> for-range-declaration while initializing an auto
> Message-ID: <ebc156f7fabfb21d6e17a50cc47abbc0 at localhost.localdomain>
> Content-Type: text/plain; charset="utf-8"
>
> Richard,
>
> thank you for feedback. Briefly, changes since last revision:
> * Replaced warning with an error. This is going to be the only diagnostics
> emitted for this case.
> * TypeContainsAuto is removed from check; I thought `auto` (strongly)
> suggests that this is a range-based for, while an `int`, for example, is
> not that strong.
> * Pass `ForRangeInit` down to
> `ParseDeclarationAfterDeclaratorAndAttributes` to set
> `ForRangeInit::ColonLoc`; this tells parser to treat a `for` statement as a
> "range-based for".
> * Fix-it uses spelling location for replacement to handle macro expansions
>
> http://reviews.llvm.org/D3370
>
> Files:
> include/clang/Basic/DiagnosticParseKinds.td
> include/clang/Parse/Parser.h
> lib/Parse/ParseDecl.cpp
> lib/Parse/ParseStmt.cpp
> test/Parser/cxx0x-for-range.cpp
> -------------- next part --------------
> A non-text attachment was scrubbed...
> Name: D3370.9188.patch
> Type: text/x-patch
> Size: 7251 bytes
> Desc: not available
> URL: <
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/attachments/20140507/c1721db4/attachment.bin
> >
>
> ------------------------------
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
> End of cfe-commits Digest, Vol 83, Issue 154
> ********************************************
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140520/989b7cdf/attachment.html>
More information about the cfe-commits
mailing list