[LLVMdev] LLVM CreateStructGEP type assert error
Tom Stellard
tom at stellard.net
Wed Aug 20 13:43:36 PDT 2014
On Wed, Aug 20, 2014 at 08:12:58PM +0000, Lowell, Daniel wrote:
>
> cB->getType()->getPointerElementType()->dump();
>
>
> gives:
>
> %struct.RB = type opaque2189 x_idx = builder.CreateStructGEP(cB, 0);
>
Ok, sorry. I thought that would dump the full struct type. Can you dump the
struct as it is defined in LLVM IR, usually if you dump the whole module,
this will be at the top.
-Tom
>
>
> cB->dump()
>
> gives:
>
> %struct.RB addrspace(1)* %cB
> $1 = void
>
> //To undo confusion, the last cB is the function arg name. For this example I unfortunately chose the same name for my argument Value* as the argument name.
> //Also one correction. I am running LLVM 3.2 not 3.4
>
>
>
> ________________________________________
> From: Tom Stellard [tom at stellard.net]
> Sent: Wednesday, August 20, 2014 2:39 PM
> To: Lowell, Daniel
> Cc: llvmdev at cs.uiuc.edu
> Subject: Re: [LLVMdev] LLVM CreateStructGEP type assert error
>
> On Wed, Aug 20, 2014 at 05:31:26PM +0000, Lowell, Daniel wrote:
> >
> > Hi all,
> >
> > Running LLVM 3.4 to create a custom pass for OpenCL transformations. I am attempting to GEP into a struct using IRBuilder's CreateStructGEP, but I keep getting this assert:
> >
> > aoc: ../../../../../../compiler/llvm/include/llvm/Instructions.h:703: llvm::Type* llvm::checkGEPType(llvm::Type*): Assertion `Ty && "Invalid GetElementPtrInst indices for type!"' failed.
> >
>
> I've hit this assertion before, and it was because I was passing
> the wrong number of operands to the GEP instruction.
>
> Can you add
>
> cB->getType()->getPointerElementType()->dump();
> cB->dump();
>
> to your pass right before you call CreateStructGEP and show us
> what the output looks like.
>
> -Tom
>
>
> > Which I've decoded as it doesn't recognize my struct as a built in type. This is confusing since it shouldn't care what type I'm indexing into, only that I give the correct indices. Otherwise what is the point of user defined structs?
> >
> > //This part is atop my cl kernel file:
> >
> > #define BUFFER_LEN 0x100000
> > typedef struct RB{
> > unsigned int x;
> > unsigned int y;
> > int z[BUFFER_LEN];
> > unsigned int xx[BUFFER_LEN];
> > unsigned int yy[BUFFER_LEN];
> > float zz[BUFFER_LEN];
> > } RB_t;
> >
> > //The kernel sig. with the struct as the last argument:
> >
> > __kernel void ht( int iteration, global RB_t *cB){ ... }
> >
> > //My LLVM code:
> >
> > void MPASS::exPointers(Module& M, Function& F){
> > BasicBlock *first = F.begin();
> > IRBuilder<> builder(first->begin());
> > Value *cB = --(F.arg_end());
> > Value *x_idx = builder.CreateStructGEP(cB, 0);
> > ...
> > ... < more of the same>
> > ...
> > return
> > }
> >
> > Simple enough, but I can't get past the first CreateStructGEP without the assert. I thought it was a version issue, so I changed the call to:
> >
> > Value *x_idx = builder.CreateConstGEP2_32(cB, 0, 0);
> >
> > However I got the same assert. It looks like this line in Instruction.h from LLVM is producing the assert further down the stack:
> >
> > 815 Type *PtrTy = PointerType::get(checkGEPType(
> > 816 getIndexedType(Ptr->getType(), IdxList)),
> > 817 Ptr->getType()->getPointerAddressSpace());
> >
> > Resulting in the debugger producing:
> >
> > (gdb) p PtrTy
> > $19 = (llvm::Type *) 0x7fffffff63d0
> > (gdb) p PtrTy->dump()
> > <unrecognized-type> $20 = void
> >
> > Is there something special which needs to be done in order to index into a user defined struct in LLVM for OpenCL?
> >
> > -----------------------------------------------
> >
> >
> >
> >
> >
> >
> >
> > ________________________________________
> > From: llvmdev-bounces at cs.uiuc.edu [llvmdev-bounces at cs.uiuc.edu] on behalf of llvmdev-request at cs.uiuc.edu [llvmdev-request at cs.uiuc.edu]
> > Sent: Wednesday, August 20, 2014 11:23 AM
> > To: llvmdev at cs.uiuc.edu
> > Subject: LLVMdev Digest, Vol 122, Issue 56
> >
> > Send LLVMdev mailing list submissions to
> > llvmdev at cs.uiuc.edu
> >
> > To subscribe or unsubscribe via the World Wide Web, visit
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
> > or, via email, send a message with subject or body 'help' to
> > llvmdev-request at cs.uiuc.edu
> >
> > You can reach the person managing the list at
> > llvmdev-owner at cs.uiuc.edu
> >
> > When replying, please edit your Subject line so it is more specific
> > than "Re: Contents of LLVMdev digest..."
> >
> >
> > Today's Topics:
> >
> > 1. Re: [RFC] Removing static initializers for command line
> > options (Pete Cooper)
> > 2. Project DiscoPoP (Entry provided) (Zhen Li)
> > 3. Re: Functions with unnamed parameters in LLVM IR (Dan Liew)
> > 4. Re: Functions with unnamed parameters in LLVM IR (Tim Northover)
> > 5. Re: Functions with unnamed parameters in LLVM IR (Dan Liew)
> > 6. Re: Proposal for ""llvm.mem.vectorize.safelen" (Robison, Arch)
> > 7. Re: Functions with unnamed parameters in LLVM IR (Dan Liew)
> > 8. Re: Proposal for ""llvm.mem.vectorize.safelen" (Robison, Arch)
> > 9. Re: Addressing const reference in ArrayRef (David Blaikie)
> > 10. Re: Proposal for ""llvm.mem.vectorize.safelen"
> > (Arnold Schwaighofer)
> > 11. Re: [RFC] Removing static initializers for command line
> > options (Rafael Esp?ndola)
> > 12. Re: Proposal for ""llvm.mem.vectorize.safelen" (Renato Golin)
> > 13. Re: [RFC] Removing static initializers for command line
> > options (Pete Cooper)
> > 14. Re: Proposal for ""llvm.mem.vectorize.safelen" (Robison, Arch)
> > 15. Re: Proposal for ""llvm.mem.vectorize.safelen" (Johannes Doerfert)
> >
> >
> > ----------------------------------------------------------------------
> >
> > Message: 1
> > Date: Tue, 19 Aug 2014 23:21:14 -0700
> > From: Pete Cooper <peter_cooper at apple.com>
> > To: Chandler Carruth <chandlerc at google.com>
> > Cc: LLVM Dev <llvmdev at cs.uiuc.edu>
> > Subject: Re: [LLVMdev] [RFC] Removing static initializers for command
> > line options
> > Message-ID: <272A2B77-7B80-44DE-B111-3DB72BACC63B at apple.com>
> > Content-Type: text/plain; charset="utf-8"
> >
> >
> > > On Aug 19, 2014, at 10:24 PM, Chandler Carruth <chandlerc at google.com> wrote:
> > >
> > >
> > > On Tue, Aug 19, 2014 at 10:10 PM, Pete Cooper <peter_cooper at apple.com> wrote:
> > >> On Aug 19, 2014, at 6:45 PM, Chandler Carruth <chandlerc at google.com> wrote:
> > >
> > >
> > >> FWIW, I largely agree with Rafael's position here, at least in the near term.
> > >>
> > >> The nice thing about it is that even if we don't stay there forever, it is a nice incremental improvement over the current state of the world, and we can actually be mindful going forward of whether the restriction it imposes (an inability to use "internal" knobs from within a library context that have multiple different library users in a single address space) proves to be a significant on-going burden.
> > > I actually disagree with this for two reasons.
> > >
> > > The first is that if there are going to be changes to the code anyway to remove static initializers, and we can move the storage to the pass at the same time, the we should make just one change and not two.
> > >
> > > No one is suggesting otherwise that I have seen? At least, my interpretation (perhaps incorrect, I've not had time to read all of this thread in 100% detail) was that the removal of static initializers wouldn't require changing every cl::opt variable.
> > It won?t no. The majority of static initializers are globals in passes, but cl::opt?s and other globals in tools for example are out of scope for this work right now (there?s no opt.cpp in a dylib so we don?t care about its globals for example).
> > >
> > >
> > > The second reason is that in most cases these knobs should not be globals. If I had a piece of data (not a CL::opt) in global scope, only used by one pass, then I'm sure people would question why it's a global at all and move it inside the class. We're treating cl::opt as special here when there's no reason for the opt or the storage to be global.
> > >
> > > We frown upon the use of globals, otherwise LLVM would be littered with them like many other C++ code bases. I don't think cl::opts should be special at all in this respect.
> > >
> > > Sure, you're arguing against the core design of cl::opt. However, we have it, and it wasn't an accident or for lack of other patterns that we chose it.
> > Oh yeah, its a fine solution for a tricky problem, but now that we?re having this discussion its clear that its use of static initializers is itself a problem.
> > >
> > > For example, we don't require all constants to be per-pass, and instead have per-file constants. Rafael is suggesting that one use case for cl::opt global variables is, in essence, a constant that is somewhat easier for a developer of LLVM (*not* a user) to change during debugging and active development. I don't think the desire for convenience and only supporting the default value in production contexts are completely invalid.
> > I can see what you?re saying here, but i?m not convinced that changing the value of a constant in global scope is any easier than changing its value in the pass constructor.
> > >
> > > Once you factor those in, we have a tradeoff. Historically, the tradeoff was made in the direction of convenience, relying on a very narrow interpretation of the use cases. It isn't clear to me that we should, today, make a different tradeoff. It certainly doesn't make sense why you would gate removing static initializers (a clear win) on forcing a change on a core design pattern within LLVM which not all of the developers are really supportive of (at this point).
> > I agree here. There?s not a strict need to move the option storage for something like an unsigned in to a pass using it (there may be for a std::string for which we?d again have a static initializer). However, I do think its the right thing to do in terms of coding guidelines. If the code to get and set an option is already in the pass initializer/constructor respectively, then I don?t think the storage should have to live outside the pass just to minimize a patch.
> >
> > Now saying this, I don?t think if the community agreed to this proposal as is, that it would mean blanket approval to change all cl::opts in all cases. But I think where its an easy change, and obviously the right choice, that options as well as their storage should be allowed to be moved in to the pass. If it makes sense to move only the option but not the storage then that should be done as a first step, and a discussion started on the right thing for the storage.
> >
> > Thanks,
> > Pete
> >
> > -------------- next part --------------
> > An HTML attachment was scrubbed...
> > URL: <http://lists.cs.uiuc.edu/pipermail/llvmdev/attachments/20140819/d25e9982/attachment-0001.html>
> >
> > ------------------------------
> >
> > Message: 2
> > Date: Wed, 20 Aug 2014 12:42:48 +0200
> > From: Zhen Li <z.li at grs-sim.de>
> > To: <llvmdev at cs.uiuc.edu>
> > Cc: discopop at grs-sim.de
> > Subject: [LLVMdev] Project DiscoPoP (Entry provided)
> > Message-ID: <1c9515d7-71de-45e0-831a-9b84d69f6de8 at HUB1.rwth-ad.de>
> > Content-Type: text/plain; charset="utf-8"; Format="flowed"
> >
> > Dear LLVM Devs,
> >
> > We are a team from German Research School for Simulation Sciences. We
> > are currently working on DiscoPoP (_Disco_very of _Po_tential
> > _P_arallelism), which is an LLVM-based tool that assists in identifying
> > potential parallelism in sequential programs. The tool uses LLVM to
> > instrument the code for finding both control and data dependences. A
> > series of analyses are built on top of dependence to explore potential
> > parallelism and parallel patterns. We have a modified version of Clang
> > with a new option "-dp" to invoke our tool.
> >
> > We have a web page about DiscoPoP: http://www.grs-sim.de/discopop, and
> > we would like to know if it is possible to have our project listed on
> > "Projects built with LLVM" page of LLVM website. Our plan is to make
> > DiscoPoP an open-source tool in the near future.
> >
> > ----------------------------------
> > The entry for our project:
> >
> > DiscoPoP: A parallelism discovery tool
> >
> > By the <a
> > href="http://www.grs-sim.de/research/parallel-programming/multicore-programming/">DiscoPoP
> > Dev Team</a>
> >
> > <a href="http://www.grs-sim.de/discopop">DiscoPoP (_Disco_very of
> > _Po_tential _P_arallelism)</a> is a tool that assists in identifying
> > potential parallelism in sequential C/C++ programs. It instruments the
> > code for finding both control and data dependences. A series of analyses
> > are built on top of dependence to explore potential parallelism and
> > parallel patterns.
> >
> > The instrumentation is done with the help of LLVM. A modified version of
> > Clang with a new option "-dp" is also provided to invoke DiscoPoP.
> > ----------------------------------
> >
> > Thank you for your attention and we are looking forward to hearing from you.
> >
> > Best regards,
> > DiscoPoP Dev Team
> > -------------- next part --------------
> > An HTML attachment was scrubbed...
> > URL: <http://lists.cs.uiuc.edu/pipermail/llvmdev/attachments/20140820/e6108f0c/attachment-0001.html>
> >
> > ------------------------------
> >
> > Message: 3
> > Date: Wed, 20 Aug 2014 15:12:44 +0100
> > From: Dan Liew <dan at su-root.co.uk>
> > To: Tim Northover <t.p.northover at gmail.com>
> > Cc: LLVM Developers Mailing List <llvmdev at cs.uiuc.edu>
> > Subject: Re: [LLVMdev] Functions with unnamed parameters in LLVM IR
> > Message-ID:
> > <CAJ7DczHn2WQkQow2yw_CuQizFx0bef1Ow4+6Ck8rPfDgA8OVPQ at mail.gmail.com>
> > Content-Type: text/plain; charset="utf-8"
> >
> > Sorry for the delay on this. Is the attached patch any better?
> >
> > I've modified the note in the Identifiers sections and trimmed down my
> > addition to the Functions section.
> >
> > I've been a little inconsistent with using argument/parameter but the
> > entire document seems to be like this so I figured it would be okay.
> >
> > Thanks,
> > Dan.
> > -------------- next part --------------
> > A non-text attachment was scrubbed...
> > Name: [v2]0001-Add-note-to-LangRef-about-how-function-arguments-can.patch
> > Type: text/x-patch
> > Size: 1713 bytes
> > Desc: not available
> > URL: <http://lists.cs.uiuc.edu/pipermail/llvmdev/attachments/20140820/9241d580/attachment-0001.bin>
> >
> > ------------------------------
> >
> > Message: 4
> > Date: Wed, 20 Aug 2014 15:17:08 +0100
> > From: Tim Northover <t.p.northover at gmail.com>
> > To: Dan Liew <dan at su-root.co.uk>
> > Cc: LLVM Developers Mailing List <llvmdev at cs.uiuc.edu>
> > Subject: Re: [LLVMdev] Functions with unnamed parameters in LLVM IR
> > Message-ID:
> > <CAFHTzfLJEuj4jAS1JZSgbvoaV7A7BvZYP5nL1SmHUiQArKXwtA at mail.gmail.com>
> > Content-Type: text/plain; charset=UTF-8
> >
> > Hi Dan,
> >
> > On 20 August 2014 15:12, Dan Liew <dan at su-root.co.uk> wrote:
> > > Sorry for the delay on this. Is the attached patch any better?
> >
> > Yep, that looks fine to me. Would you like me to commit it or do you
> > have access?
> >
> > Cheers.
> >
> > Tim.
> >
> >
> > ------------------------------
> >
> > Message: 5
> > Date: Wed, 20 Aug 2014 15:51:04 +0100
> > From: Dan Liew <dan at su-root.co.uk>
> > To: Tim Northover <t.p.northover at gmail.com>
> > Cc: LLVM Developers Mailing List <llvmdev at cs.uiuc.edu>
> > Subject: Re: [LLVMdev] Functions with unnamed parameters in LLVM IR
> > Message-ID:
> > <CAJ7DczF_VbSnUwhS-vWNf_DWUzt5ybcgoBS3L0+GObF_Cokb8w at mail.gmail.com>
> > Content-Type: text/plain; charset=UTF-8
> >
> > > On 20 August 2014 15:12, Dan Liew <dan at su-root.co.uk> wrote:
> > >> Sorry for the delay on this. Is the attached patch any better?
> > >
> > > Yep, that looks fine to me. Would you like me to commit it or do you
> > > have access?
> >
> > Thanks for taking a look. I have commit access so I'll commit it later on today.
> >
> > Thanks,
> > Dan.
> >
> >
> > ------------------------------
> >
> > Message: 6
> > Date: Wed, 20 Aug 2014 15:08:15 +0000
> > From: "Robison, Arch" <arch.robison at intel.com>
> > To: Hal Finkel <hfinkel at anl.gov>, Roel Jordans <r.jordans at tue.nl>
> > Cc: "llvmdev at cs.uiuc.edu" <llvmdev at cs.uiuc.edu>
> > Subject: Re: [LLVMdev] Proposal for ""llvm.mem.vectorize.safelen"
> > Message-ID:
> > <51C2718BFE68DA4698E41A5C59B2BB69574200FB at fmsmsx116.amr.corp.intel.com>
> >
> > Content-Type: text/plain; charset="utf-8"
> >
> > > I recommend that you send patches for an implementation
> > > (including the Loop::GetAnnotatedVectorSafelen function
> > > and associated updates to the vectorizer) for review.
> >
> > I expect to send the patches for review later this week.
> >
> > > Make sure to remember LangRef updates!
> >
> > Thanks for the reminder.
> >
> > > Also, looking at the original proposal again, I see no reason
> > > to restrict this to an i32: we might as well allow it to be an
> > > i64 obviously we don't have billion-lane vectors, but the
> > > metadata can also be useful for other kinds of analysis).
> >
> > I doubt there is much advantage between knowing the minimum loop-carried
> > dependence difference is 1u<<31 or 1u<<63. Unless there is a clear
> > need for the higher values less than infinity, It would seem simpler
> > for now to adopt the same conventions as llvm.loop.vectorize.width
> > so that some processing infrastructure can be shared easily.
> >
> > > I don't like using the name 'safelen' however. I can forgive OpenMP
> > > for choosing such a short name because people need to type it,
> > > but I'd prefer that the metadata have a more-descriptive name.
> > > minimum_dependency_distance is perhaps better.
> >
> > I'm open to naming suggestions, though I'm wondering if sticking with
> > what is now a term of art in OpenMP might be the least of all evils,
> > because the semantics turn out to be a little more subtle than
> > just a minimum dependence distance. My current wording of the semantics
> > for safelen of k are:
> >
> > /// The loop can be assumed to have no loop-carried
> > /// lexically backward dependencies with distance less than k.
> >
> > - Arch D. Robison
> > Intel Corporation
> >
> >
> >
> >
> > ------------------------------
> >
> > Message: 7
> > Date: Wed, 20 Aug 2014 16:16:31 +0100
> > From: Dan Liew <dan at su-root.co.uk>
> > To: Tim Northover <t.p.northover at gmail.com>
> > Cc: LLVM Developers Mailing List <llvmdev at cs.uiuc.edu>
> > Subject: Re: [LLVMdev] Functions with unnamed parameters in LLVM IR
> > Message-ID:
> > <CAJ7DczHz_bb__ATrFOB8wgKDkTVqRR3-Zs0GdhCS35+unUCB+A at mail.gmail.com>
> > Content-Type: text/plain; charset=UTF-8
> >
> > Committed in r216070
> >
> >
> > ------------------------------
> >
> > Message: 8
> > Date: Wed, 20 Aug 2014 15:17:01 +0000
> > From: "Robison, Arch" <arch.robison at intel.com>
> > To: Hal Finkel <hfinkel at anl.gov>, Roel Jordans <r.jordans at tue.nl>
> > Cc: "llvmdev at cs.uiuc.edu" <llvmdev at cs.uiuc.edu>
> > Subject: Re: [LLVMdev] Proposal for ""llvm.mem.vectorize.safelen"
> > Message-ID:
> > <51C2718BFE68DA4698E41A5C59B2BB6957420167 at fmsmsx116.amr.corp.intel.com>
> >
> > Content-Type: text/plain; charset="utf-8"
> >
> > Now that I'm looking at editing LangRef, I have a question. The current
> > llvm.loop.vectorize metadata are hints, and have this constraint:
> >
> > The``llvm.loop.vectorize`` and ``llvm.loop.interleave`` metadata are only
> > optimization hints and the optimizer will only interleave and vectorize loops
> > if it believes it is safe to do so.
> >
> > But safelen is different. It's an assertion about safety, so prefixing it with
> > llvm.loop.vectorize seems inappropriate. Does is sound reasonable to drop
> > the "vectorize" portion. Maybe spell it something like this?
> >
> > llvm.loop.carried_dependence_distance.min
> >
> > - Arch
> >
> >
> >
> >
> > ------------------------------
> >
> > Message: 9
> > Date: Wed, 20 Aug 2014 08:28:37 -0700
> > From: David Blaikie <dblaikie at gmail.com>
> > To: Joey Ye <joey.ye.cc at gmail.com>
> > Cc: LLVM Developers Mailing List <llvmdev at cs.uiuc.edu>
> > Subject: Re: [LLVMdev] Addressing const reference in ArrayRef
> > Message-ID:
> > <CAENS6EveGNodi4ok3dJNzOnmkdwLmqOHyw07MrgO-u4FAB6KLQ at mail.gmail.com>
> > Content-Type: text/plain; charset="utf-8"
> >
> > I seem to recall discussing this before - is there an existing llvm bug
> > filed, another email thread or something (or perhaps it was just an IRC
> > conversation)? It would be good to keep all the discussion together or at
> > least reference the prior (llvm community) discussion.
> >
> > Have you tried applying your suggested patch? I assume you meant to suggest
> > replacing/modifying the existing ctor (to take non-const ref) rather than
> > adding another?
> >
> > I'd assume that causes a lot of build failures as we probably rely on
> > binding temporaries to ArrayRef's in many places correctly (most ArrayRef's
> > are temporaries, not local variables).
> >
> > I think in the previous discussion I suggested we might just want to make a
> > practice of treating named/local (not parameter) ArrayRef's with greater
> > suspicion, the same way we do for twine, but I'm not sure.
> >
> > We could move this ctor into a factory function (and/or just make the CTor
> > private and friend the makeArrayRef helper for this case) to make it more
> > obvious/easier to find bad call sites. But it would be helpful to have the
> > context of the prior discussion to continue from there.
> > On Aug 19, 2014 11:18 PM, "Joey Ye" <joey.ye.cc at gmail.com> wrote:
> >
> > > Analyzing why GCC failed to build LLVM recently, one root cause lies
> > > in definition of ArrayRef:
> > > // ArrayRef.h:
> > > ArrayRef(const T &OneElt) : Data(&OneElt), Length(1) {}
> > >
> > > Here address of const reference is taken and stored to an object. It
> > > is believed that live range of const reference is only at the function
> > > call site, escaping of its address to an object with a longer live
> > > range is invalid. Referring to the case and discussion here:
> > > https://gcc.gnu.org/ml/gcc/2014-08/msg00173.html
> > >
> > > So I would suggest to fix ArrayRef. Adding a non-const version of
> > > constructor should work, but it still leaves the vulnerability in
> > > const version, which I'd expect on people in the community to work out
> > > a solution.
> > > ArrayRef(T &OneElt) : Data(&OneElt), Length(1) {}
> > >
> > >
> > > Thanks,
> > > Joey
> > > _______________________________________________
> > > LLVM Developers mailing list
> > > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
> > >
> > -------------- next part --------------
> > An HTML attachment was scrubbed...
> > URL: <http://lists.cs.uiuc.edu/pipermail/llvmdev/attachments/20140820/55b6ad6e/attachment-0001.html>
> >
> > ------------------------------
> >
> > Message: 10
> > Date: Wed, 20 Aug 2014 08:29:48 -0700
> > From: Arnold Schwaighofer <aschwaighofer at apple.com>
> > To: "Robison, Arch" <arch.robison at intel.com>
> > Cc: "llvmdev at cs.uiuc.edu" <llvmdev at cs.uiuc.edu>
> > Subject: Re: [LLVMdev] Proposal for ""llvm.mem.vectorize.safelen"
> > Message-ID: <6EDAADA1-F702-4AAD-BF58-0A1BFB640461 at apple.com>
> > Content-Type: text/plain; charset=utf-8
> >
> >
> > > On Aug 20, 2014, at 8:08 AM, Robison, Arch <arch.robison at intel.com> wrote:
> > >
> > >> I recommend that you send patches for an implementation
> > >> (including the Loop::GetAnnotatedVectorSafelen function
> > >> and associated updates to the vectorizer) for review.
> > >
> > > I expect to send the patches for review later this week.
> > >
> > >> Make sure to remember LangRef updates!
> > >
> > > Thanks for the reminder.
> > >
> > >> Also, looking at the original proposal again, I see no reason
> > >> to restrict this to an i32: we might as well allow it to be an
> > >> i64 obviously we don't have billion-lane vectors, but the
> > >> metadata can also be useful for other kinds of analysis).
> > >
> > > I doubt there is much advantage between knowing the minimum loop-carried
> > > dependence difference is 1u<<31 or 1u<<63. Unless there is a clear
> > > need for the higher values less than infinity, It would seem simpler
> > > for now to adopt the same conventions as llvm.loop.vectorize.width
> > > so that some processing infrastructure can be shared easily.
> > >
> > >> I don't like using the name 'safelen' however. I can forgive OpenMP
> > >> for choosing such a short name because people need to type it,
> > >> but I'd prefer that the metadata have a more-descriptive name.
> > >> minimum_dependency_distance is perhaps better.
> > >
> > > I'm open to naming suggestions, though I'm wondering if sticking with
> > > what is now a term of art in OpenMP might be the least of all evils,
> > > because the semantics turn out to be a little more subtle than
> > > just a minimum dependence distance. My current wording of the semantics
> > > for safelen of k are:
> > >
> > > /// The loop can be assumed to have no loop-carried
> > > /// lexically backward dependencies with distance less than k.
> >
> > This means you would allow lexically forward dependencies which complicates things (this would need more infrastructure in clang). You need to carry over ?source order? from the front-end. Because the dependency is loop carried llvm would be allowed to move loads and stores within the loop:
> >
> > Lexical forward dependency:
> >
> > #pragma vectorize safelen(4)
> > for (i = ?) {
> > a[i] = b[i] + z[i];
> > c[i] = a[i-1] + 1;
> > }
> >
> > LLVM might tranform this loop to:
> >
> > for (i = ?) {
> > c[i] = a[i-1] + 1;
> > a[i] = b[i] + z[i];
> > }
> >
> > if we now vectorize this (without knowledge of the orignal source order) we get the wrong answer:
> >
> > for (i = ?) {
> > c[i] = a[i-1:i+2] + 1;
> > a[i:i+3] = b[i] + z[i];
> > }
> >
> > Alternatively, the metadata loop.vectorize.safelen would have to prevent any such reordering of accesses i.e. any pass that reorders memory accesses would have to be aware of it which is fragile.
> >
> > >
> > > - Arch D. Robison
> > > Intel Corporation
> > >
> > >
> > > _______________________________________________
> > > LLVM Developers mailing list
> > > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
> >
> >
> >
> >
> > ------------------------------
> >
> > Message: 11
> > Date: Wed, 20 Aug 2014 11:51:56 -0400
> > From: Rafael Esp?ndola <rafael.espindola at gmail.com>
> > To: Pete Cooper <peter_cooper at apple.com>
> > Cc: LLVM Dev <llvmdev at cs.uiuc.edu>
> > Subject: Re: [LLVMdev] [RFC] Removing static initializers for command
> > line options
> > Message-ID:
> > <CAG3jReKcN9niyhGsfZrHg2qFajVvpd+wyVE9MQokydMU-0SNoA at mail.gmail.com>
> > Content-Type: text/plain; charset=UTF-8
> >
> > > FWIW, I largely agree with Rafael's position here, at least in the near
> > > term.
> > >
> > > The nice thing about it is that even if we don't stay there forever, it is a
> > > nice incremental improvement over the current state of the world, and we can
> > > actually be mindful going forward of whether the restriction it imposes (an
> > > inability to use "internal" knobs from within a library context that have
> > > multiple different library users in a single address space) proves to be a
> > > significant on-going burden.
> > >
> > > I actually disagree with this for two reasons.
> > >
> > > The first is that if there are going to be changes to the code anyway to
> > > remove static initializers, and we can move the storage to the pass at the
> > > same time, the we should make just one change and not two.
> > >
> > > The second reason is that in most cases these knobs should not be globals.
> > > If I had a piece of data (not a CL::opt) in global scope, only used by one
> > > pass, then I'm sure people would question why it's a global at all and move
> > > it inside the class. We're treating cl::opt as special here when there's no
> > > reason for the opt or the storage to be global.
> > >
> > > We frown upon the use of globals, otherwise LLVM would be littered with them
> > > like many other C++ code bases. I don't think cl::opts should be special at
> > > all in this respect.
> >
> >
> > Note that the call
> >
> > cl::OptionRegistry::CreateOption<bool>(&ScalarizeLoadStore,
> > "ScalarizeLoadStore",
> > "scalarize-load-store", cl::Hidden, cl::init(false),
> > cl::desc("Allow the scalarizer pass to scalarize loads and store"));
> >
> > ScalarizeLoadStore can actually be a member variable as long as caller
> > guarantees it is still around when the command line is parsed. I have
> > no objections to doing this move in the first pass if you want to.
> >
> > What I would really like to avoid for now is the
> >
> > cl::OptionRegistry::GetValue<bool>("ScalarizeLoadStore");
> >
> > Cheers,
> > Rafael
> >
> >
> > ------------------------------
> >
> > Message: 12
> > Date: Wed, 20 Aug 2014 17:00:07 +0100
> > From: Renato Golin <renato.golin at linaro.org>
> > To: "Robison, Arch" <arch.robison at intel.com>
> > Cc: "llvmdev at cs.uiuc.edu" <llvmdev at cs.uiuc.edu>
> > Subject: Re: [LLVMdev] Proposal for ""llvm.mem.vectorize.safelen"
> > Message-ID:
> > <CAMSE1kd9S41Oy3hxXBMz2r0Mz68JKPu+tukunONnEMiQBOQZNg at mail.gmail.com>
> > Content-Type: text/plain; charset=UTF-8
> >
> > On 20 August 2014 16:17, Robison, Arch <arch.robison at intel.com> wrote:
> > > But safelen is different. It's an assertion about safety, so prefixing it with
> > > llvm.loop.vectorize seems inappropriate. Does is sound reasonable to drop
> > > the "vectorize" portion. Maybe spell it something like this?
> >
> > This was true to all *current* vectorizer pragmas, but certainly not
> > safelen. I think we can change that description instead of changing
> > the namespace.
> >
> > cheers,
> > --renato
> >
> >
> > ------------------------------
> >
> > Message: 13
> > Date: Wed, 20 Aug 2014 09:15:02 -0700
> > From: Pete Cooper <peter_cooper at apple.com>
> > To: Rafael Esp?ndola <rafael.espindola at gmail.com>
> > Cc: LLVM Dev <llvmdev at cs.uiuc.edu>
> > Subject: Re: [LLVMdev] [RFC] Removing static initializers for command
> > line options
> > Message-ID: <154097CE-BB97-4746-B381-CD95706873D1 at apple.com>
> > Content-Type: text/plain; charset=utf-8
> >
> >
> > > On Aug 20, 2014, at 8:51 AM, Rafael Esp?ndola <rafael.espindola at gmail.com> wrote:
> > >
> > >> FWIW, I largely agree with Rafael's position here, at least in the near
> > >> term.
> > >>
> > >> The nice thing about it is that even if we don't stay there forever, it is a
> > >> nice incremental improvement over the current state of the world, and we can
> > >> actually be mindful going forward of whether the restriction it imposes (an
> > >> inability to use "internal" knobs from within a library context that have
> > >> multiple different library users in a single address space) proves to be a
> > >> significant on-going burden.
> > >>
> > >> I actually disagree with this for two reasons.
> > >>
> > >> The first is that if there are going to be changes to the code anyway to
> > >> remove static initializers, and we can move the storage to the pass at the
> > >> same time, the we should make just one change and not two.
> > >>
> > >> The second reason is that in most cases these knobs should not be globals.
> > >> If I had a piece of data (not a CL::opt) in global scope, only used by one
> > >> pass, then I'm sure people would question why it's a global at all and move
> > >> it inside the class. We're treating cl::opt as special here when there's no
> > >> reason for the opt or the storage to be global.
> > >>
> > >> We frown upon the use of globals, otherwise LLVM would be littered with them
> > >> like many other C++ code bases. I don't think cl::opts should be special at
> > >> all in this respect.
> > >
> > >
> > > Note that the call
> > >
> > > cl::OptionRegistry::CreateOption<bool>(&ScalarizeLoadStore,
> > > "ScalarizeLoadStore",
> > > "scalarize-load-store", cl::Hidden, cl::init(false),
> > > cl::desc("Allow the scalarizer pass to scalarize loads and store"));
> > >
> > > ScalarizeLoadStore can actually be a member variable as long as caller
> > > guarantees it is still around when the command line is parsed. I have
> > > no objections to doing this move in the first pass if you want to.
> > >
> > > What I would really like to avoid for now is the
> > >
> > > cl::OptionRegistry::GetValue<bool>("ScalarizeLoadStore?);
> > Ah, I totally see what you mean now. Sorry if i had been confused before.
> >
> > To be honest, I had exactly the same reservations myself, but then i looked in to how we initialize and create passes and there actually isn?t a nice way to do this right now.
> >
> > The trouble is that INITIALIZE_PASS doesn?t actually construct a pass. It actually constructs a PassInfo which has a pointer to the default constructor of the pass. Later, if the pass manager needs to (say -scalarize was on the commandline), it will get the default constructor from PassInfo and construct the pass.
> >
> > Unfortunately, this completely detaches the lifetime of the pass instance where we want to store the ScalarizeLoadStore bool, from the option code to hook in to the cl::opt stuff. I originally wanted to do something gross like pass __offset_of(Scalarizer::ScalarizeLoadStore) to the PassInfo and hide the initialization of the option in there. But that doesn?t work either, as there?s no guarantee you?ll even create a instance of Scalarizer, even though you could pass -scalarize-load-store to the command line.
> >
> > So, we do have 2 solutions: a global variable, and a call to GetValue. As you can see, either way isn?t perfect, but if you can think of another solution i?m sure we can discuss it.
> >
> > Thanks,
> > Pete
> > >
> > > Cheers,
> > > Rafael
> >
> >
> >
> >
> > ------------------------------
> >
> > Message: 14
> > Date: Wed, 20 Aug 2014 16:16:32 +0000
> > From: "Robison, Arch" <arch.robison at intel.com>
> > To: Arnold Schwaighofer <aschwaighofer at apple.com>
> > Cc: "llvmdev at cs.uiuc.edu" <llvmdev at cs.uiuc.edu>
> > Subject: Re: [LLVMdev] Proposal for ""llvm.mem.vectorize.safelen"
> > Message-ID:
> > <51C2718BFE68DA4698E41A5C59B2BB69574201AB at fmsmsx116.amr.corp.intel.com>
> >
> > Content-Type: text/plain; charset="utf-8"
> >
> > Thanks for alerting me to this issue. This is going to take more
> > effort than I thought :-(. Given that a major motivation behind
> > the OpenMP #pragma omp simd is to allow lexical forward dependences,
> > we should find a way to do this right.
> >
> > I agree that we want to avoid making passes that reorder accesses fragile.
> > The extra work in Clang (or Julia :-) seems unavoidable, unless those
> > producers always emit LLVM instructions in lexical order. If the latter
> > is the case, perhaps we could have a helper routine that, given the IR early
> > and in lexical order, finishes the annotation work?
> >
> > It seems that we need metadata on each memory access, distinct from
> > llvm.mem.parallel_loop_access. Say something like llvm.mem.vector_loop_access
> > that includes relative lexical position as a third operand? Second operand
> > could point back to the metadata with the minimum loop-carried distance,
> > which in turn could point back to the loop id.
> >
> > - Arch
> >
> >
> >
> >
> >
> > ------------------------------
> >
> > Message: 15
> > Date: Wed, 20 Aug 2014 09:18:14 -0700
> > From: Johannes Doerfert <doerfert at cs.uni-saarland.de>
> > To: Arnold Schwaighofer <aschwaighofer at apple.com>
> > Cc: "llvmdev at cs.uiuc.edu" <llvmdev at cs.uiuc.edu>, "Robison, Arch"
> > <arch.robison at intel.com>
> > Subject: Re: [LLVMdev] Proposal for ""llvm.mem.vectorize.safelen"
> > Message-ID: <20140820161814.GD4039 at JD-Arch>
> > Content-Type: text/plain; charset="utf-8"
> >
> > On 08/20, Arnold Schwaighofer wrote:
> > >
> > > > On Aug 20, 2014, at 8:08 AM, Robison, Arch <arch.robison at intel.com> wrote:
> > > >
> > > >> I recommend that you send patches for an implementation
> > > >> (including the Loop::GetAnnotatedVectorSafelen function
> > > >> and associated updates to the vectorizer) for review.
> > > >
> > > > I expect to send the patches for review later this week.
> > > >
> > > >> Make sure to remember LangRef updates!
> > > >
> > > > Thanks for the reminder.
> > > >
> > > >> Also, looking at the original proposal again, I see no reason
> > > >> to restrict this to an i32: we might as well allow it to be an
> > > >> i64 obviously we don't have billion-lane vectors, but the
> > > >> metadata can also be useful for other kinds of analysis).
> > > >
> > > > I doubt there is much advantage between knowing the minimum loop-carried
> > > > dependence difference is 1u<<31 or 1u<<63. Unless there is a clear
> > > > need for the higher values less than infinity, It would seem simpler
> > > > for now to adopt the same conventions as llvm.loop.vectorize.width
> > > > so that some processing infrastructure can be shared easily.
> > > >
> > > >> I don't like using the name 'safelen' however. I can forgive OpenMP
> > > >> for choosing such a short name because people need to type it,
> > > >> but I'd prefer that the metadata have a more-descriptive name.
> > > >> minimum_dependency_distance is perhaps better.
> > > >
> > > > I'm open to naming suggestions, though I'm wondering if sticking with
> > > > what is now a term of art in OpenMP might be the least of all evils,
> > > > because the semantics turn out to be a little more subtle than
> > > > just a minimum dependence distance. My current wording of the semantics
> > > > for safelen of k are:
> > > >
> > > > /// The loop can be assumed to have no loop-carried
> > > > /// lexically backward dependencies with distance less than k.
> > >
> > > This means you would allow lexically forward dependencies which complicates things (this would need more infrastructure in clang). You need to carry over ?source order? from the front-end. Because the dependency is loop carried llvm would be allowed to move loads and stores within the loop:
> > This should not be that hard (see below).
> >
> > > Lexical forward dependency:
> > >
> > > #pragma vectorize safelen(4)
> > > for (i = ?) {
> > > a[i] = b[i] + z[i];
> > > c[i] = a[i-1] + 1;
> > > }
> > >
> > > LLVM might tranform this loop to:
> > >
> > > for (i = ?) {
> > > c[i] = a[i-1] + 1;
> > > a[i] = b[i] + z[i];
> > > }
> > >
> > > if we now vectorize this (without knowledge of the orignal source order) we get the wrong answer:
> > >
> > > for (i = ?) {
> > > c[i] = a[i-1:i+2] + 1;
> > > a[i:i+3] = b[i] + z[i];
> > > }
> > >
> > > Alternatively, the metadata loop.vectorize.safelen would have to prevent any such reordering of accesses i.e. any pass that reorders memory accesses would have to be aware of it which is fragile.
> > Could we number the memory accesses for such loops (e.g., in clang)?
> > Adding metadata to each memory access which points to the safelen
> > annotation and contains an increasing constant would be similarly
> > fragile as other constructions we use at the moment. However, it would
> > allow us to determine iff the current order is still the original one or
> > not. (At least if I did not miss anything).
> >
> > Best regards,
> > Johannes
> >
> > --
> >
> > Johannes Doerfert
> > Researcher / PhD Student
> >
> > Compiler Design Lab (Prof. Hack)
> > Saarland University, Computer Science
> > Building E1.3, Room 4.26
> >
> > Tel. +49 (0)681 302-57521 : doerfert at cs.uni-saarland.de
> > Fax. +49 (0)681 302-3065 : http://www.cdl.uni-saarland.de/people/doerfert
> > -------------- next part --------------
> > A non-text attachment was scrubbed...
> > Name: not available
> > Type: application/pgp-signature
> > Size: 213 bytes
> > Desc: not available
> > URL: <http://lists.cs.uiuc.edu/pipermail/llvmdev/attachments/20140820/12b3dcf1/attachment.sig>
> >
> > ------------------------------
> >
> > _______________________________________________
> > LLVMdev mailing list
> > LLVMdev at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
> >
> >
> > End of LLVMdev Digest, Vol 122, Issue 56
> > ****************************************
> > _______________________________________________
> > LLVM Developers mailing list
> > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
More information about the llvm-dev
mailing list