On Feb 12, 2013, at 9:02 AM, Tobias Grosser <tobias at grosser.es> wrote:> On 02/12/2013 05:54 PM, Nadav Rotem wrote: >> >>> I have the feeling option 2) does not work for you, but I don't yet understand your reasons. >> >> My inclination to prefer #1 is due to its simplicity. But, if #1 does not work because it creates a correctness problems then #2 is the only option that is left on the table. > > Unfortunately. :-( > > Does your reply mean you agree that option 2) should be taken?Yes. I think that there is a consensus that this is the right approach.> In case you do, how should we proceed? Should Pekka submit his patch for a final pre-commit review?I think so. Pekka's patch from Jan 31st looks good to me. The is bug in the metadata handling in the vectorizer that we don't check that the load/store annotation matches the loop metadata.> > If option 2) is OK, Paul could also try to produce this metadate from his #pragma ivdep parser. >That would be wonderful. Thanks, Nadav
Can we summarize this thread and the final recommendation? :P paul On 2013-02-12, at 1:12 PM, Nadav Rotem wrote:> > On Feb 12, 2013, at 9:02 AM, Tobias Grosser <tobias at grosser.es> wrote: > >> On 02/12/2013 05:54 PM, Nadav Rotem wrote: >>> >>>> I have the feeling option 2) does not work for you, but I don't yet understand your reasons. >>> >>> My inclination to prefer #1 is due to its simplicity. But, if #1 does not work because it creates a correctness problems then #2 is the only option that is left on the table. >> >> Unfortunately. :-( >> >> Does your reply mean you agree that option 2) should be taken? > > Yes. I think that there is a consensus that this is the right approach. > >> In case you do, how should we proceed? Should Pekka submit his patch for a final pre-commit review? > > I think so. Pekka's patch from Jan 31st looks good to me. The is bug in the metadata handling in the vectorizer that we don't check that the load/store annotation matches the loop metadata. > >> >> If option 2) is OK, Paul could also try to produce this metadate from his #pragma ivdep parser. >> > > That would be wonderful. > > Thanks, > Nadav > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Hi, Here it is, just synched against the latest LLVM trunk. Shall I commit this now? After committing, it could be worth some planning what is the best way to provide an easy to use mechanism to "refresh" the parallel loop mem access metadata (llvm.mem.parallel_loop_access) after optimizations that do not render the loop to a serial one. Some kind of helper function should be added to make it as easy as possible. E.g., if the inliner inlines a call to a parallel loop's body, how can it easily and robustly refresh the parallel loop metadata to the inlined function? It needs to annotate only the new memory instructions in the loop with the parallel mem metadata, but not any of the old ones as there might have been a parallel-loop-unaware pass before that has added mem instructions without the metadata. Thus, we cannot just blindly refresh the whole parallel loop with the parallel mem access metadata after the pass finishes. On 02/12/2013 08:12 PM, Nadav Rotem wrote:> > On Feb 12, 2013, at 9:02 AM, Tobias Grosser <tobias at grosser.es> wrote: > >> On 02/12/2013 05:54 PM, Nadav Rotem wrote: >>> >>>> I have the feeling option 2) does not work for you, but I don't yet >>>> understand your reasons. >>> >>> My inclination to prefer #1 is due to its simplicity. But, if #1 does not >>> work because it creates a correctness problems then #2 is the only option >>> that is left on the table. >> >> Unfortunately. :-( >> >> Does your reply mean you agree that option 2) should be taken? > > Yes. I think that there is a consensus that this is the right approach. > >> In case you do, how should we proceed? Should Pekka submit his patch for a >> final pre-commit review? > > I think so. Pekka's patch from Jan 31st looks good to me. The is bug in the > metadata handling in the vectorizer that we don't check that the load/store > annotation matches the loop metadata. > >> >> If option 2) is OK, Paul could also try to produce this metadate from his >> #pragma ivdep parser. >> > > That would be wonderful. > > Thanks, Nadav >-- --Pekka -------------- next part -------------- A non-text attachment was scrubbed... Name: llvm-3.3-parallel-loop-metadata.patch Type: text/x-diff Size: 18027 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20130212/453fb427/attachment.patch>
On 02/12/2013 10:41 PM, Pekka Jääskeläinen wrote:> Hi, > > Here it is, just synched against the latest LLVM trunk. Shall I commit this > now?Looks good from my side. If there are no further remarks, I believe you can commit it.> After committing, it could be worth some planning what is the best way to > provide an easy to use mechanism to "refresh" the parallel loop mem > access metadata (llvm.mem.parallel_loop_access) after optimizations that > do not > render the loop to a serial one. Some kind of helper function should be > added to make it as easy as possible. > > E.g., if the inliner inlines a call to a parallel loop's body, how can it > easily and robustly refresh the parallel loop metadata to the inlined > function? > It needs to annotate only the new memory instructions in the loop with the > parallel mem metadata, but not any of the old ones as there might > have been a parallel-loop-unaware pass before that has added mem > instructions > without the metadata. Thus, we cannot just blindly refresh the whole > parallel > loop with the parallel mem access metadata after the pass finishes.Yes. That is an interesting question. I don't have any solution in mind yet. Tobi
----- Original Message -----> From: "Pekka Jääskeläinen" <pekka.jaaskelainen at tut.fi> > To: "Nadav Rotem" <nrotem at apple.com>, "llvmdev at cs.uiuc.edu Dev" <llvmdev at cs.uiuc.edu> > Cc: "Tobias Grosser" <tobias at grosser.es> > Sent: Tuesday, February 12, 2013 3:41:25 PM > Subject: Re: [LLVMdev] Parallel Loop Metadata > > Hi, > > Here it is, just synched against the latest LLVM trunk. Shall I > commit this > now? > > > After committing, it could be worth some planning what is the best > way to > provide an easy to use mechanism to "refresh" the parallel loop mem > access metadata (llvm.mem.parallel_loop_access) after optimizations > that do not > render the loop to a serial one. Some kind of helper function should > be > added to make it as easy as possible. > > E.g., if the inliner inlines a call to a parallel loop's body, how > can it > easily and robustly refresh the parallel loop metadata to the inlined > function? > It needs to annotate only the new memory instructions in the loop > with the > parallel mem metadata, but not any of the old ones as there might > have been a parallel-loop-unaware pass before that has added mem > instructions > without the metadata. Thus, we cannot just blindly refresh the whole > parallel > loop with the parallel mem access metadata after the pass finishes.I don't think that you're thinking about this in the right way. We need to update any relevant passes to preserve the metadata 'in situ'. If there is a pass that is dropping the metadata in an unnecessary way, then we have two situations: 1. If the pass handles TBAA metadata, then update it to handle this metadata in the same way 2. If not, think, code, refactor, etc. ;) You may just want to start by grepping the existing source tree for TBAA; that will give you some idea of what will be involved. -Hal> > On 02/12/2013 08:12 PM, Nadav Rotem wrote: > > > > On Feb 12, 2013, at 9:02 AM, Tobias Grosser <tobias at grosser.es> > > wrote: > > > >> On 02/12/2013 05:54 PM, Nadav Rotem wrote: > >>> > >>>> I have the feeling option 2) does not work for you, but I don't > >>>> yet > >>>> understand your reasons. > >>> > >>> My inclination to prefer #1 is due to its simplicity. But, if #1 > >>> does not > >>> work because it creates a correctness problems then #2 is the > >>> only option > >>> that is left on the table. > >> > >> Unfortunately. :-( > >> > >> Does your reply mean you agree that option 2) should be taken? > > > > Yes. I think that there is a consensus that this is the right > > approach. > > > >> In case you do, how should we proceed? Should Pekka submit his > >> patch for a > >> final pre-commit review? > > > > I think so. Pekka's patch from Jan 31st looks good to me. The is > > bug in the > > metadata handling in the vectorizer that we don't check that the > > load/store > > annotation matches the loop metadata. > > > >> > >> If option 2) is OK, Paul could also try to produce this metadate > >> from his > >> #pragma ivdep parser. > >> > > > > That would be wonderful. > > > > Thanks, Nadav > > > > > -- > --Pekka > > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >
On Feb 12, 2013, at 1:41 PM, Pekka Jääskeläinen <pekka.jaaskelainen at tut.fi> wrote:> Hi, > > Here it is, just synched against the latest LLVM trunk. Shall I commit this > now?LGTM. Please commit.> After committing, it could be worth some planning what is the best way to > provide an easy to use mechanism to "refresh" the parallel loop mem > access metadata (llvm.mem.parallel_loop_access) after optimizations that do not > render the loop to a serial one. Some kind of helper function should be > added to make it as easy as possible. > > E.g., if the inliner inlines a call to a parallel loop's body, how can it > easily and robustly refresh the parallel loop metadata to the inlined function? > It needs to annotate only the new memory instructions in the loop with the > parallel mem metadata, but not any of the old ones as there might > have been a parallel-loop-unaware pass before that has added mem instructions > without the metadata. Thus, we cannot just blindly refresh the whole parallel > loop with the parallel mem access metadata after the pass finishes. >Lets wait and see if this is a real problem before we change lots of passes in the compiler.> On 02/12/2013 08:12 PM, Nadav Rotem wrote: >> >> On Feb 12, 2013, at 9:02 AM, Tobias Grosser <tobias at grosser.es> wrote: >> >>> On 02/12/2013 05:54 PM, Nadav Rotem wrote: >>>> >>>>> I have the feeling option 2) does not work for you, but I don't yet >>>>> understand your reasons. >>>> >>>> My inclination to prefer #1 is due to its simplicity. But, if #1 does not >>>> work because it creates a correctness problems then #2 is the only option >>>> that is left on the table. >>> >>> Unfortunately. :-( >>> >>> Does your reply mean you agree that option 2) should be taken? >> >> Yes. I think that there is a consensus that this is the right approach. >> >>> In case you do, how should we proceed? Should Pekka submit his patch for a >>> final pre-commit review? >> >> I think so. Pekka's patch from Jan 31st looks good to me. The is bug in the >> metadata handling in the vectorizer that we don't check that the load/store >> annotation matches the loop metadata. >> >>> >>> If option 2) is OK, Paul could also try to produce this metadate from his >>> #pragma ivdep parser. >>> >> >> That would be wonderful. >> >> Thanks, Nadav >> > > > -- > --Pekka > > <llvm-3.3-parallel-loop-metadata.patch>