[LLVMdev] LLVM CreateStructGEP type assert error
Lowell, Daniel
Daniel.Lowell at amd.com
Wed Aug 20 10:31:26 PDT 2014
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.
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
****************************************
More information about the llvm-dev
mailing list