On Feb 8, 2013, at 12:28 PM, Pekka Jääskeläinen <pekka.jaaskelainen at tut.fi> wrote:> On 02/08/2013 08:20 PM, Tobias Grosser wrote: >> That sounds elegant and seems to solve the correctness problems. > > There is no big difference here except that the memory instructions > would not need the metadata. > > I do not think the abundance of metadata is really the main problem, but > what to do with passes that do not know about parallel loops. How to minimize the changes needed to make the most relevant passes retain the information, > and more importantly, how to avoid code breakage if they are unaware of > its meaning. > > Also, I tried something like this when I tried to find a nice way to couple > the memory and the loop metadata. It seems not possible to directly refer to > an Instruction from MDNode, or is it? I got crashes when serializing the > bitcode with MDNodes pointing to Instructions and thought that it's not meant > to work that way around.I think you can refer to the memory address values from within your function-local metadata.> Thus, I created the loop identifier metadata "anchor" to which the both > metadata types refer. > Even if it was possible, retaining that information across inlining etc. means > one has to update those Instruction references in the metadata to the new > matching ones in the inlined location, etc. The maintenance burden of the data > in several passes, which I thought was the actual concern of Nadav, gets AFAIU > even harder. E.g., if you remove memory instructions (e.g. DCE) you get > dangling pointers in the metadata or a null there which renders the parallel info invalid (and might even cause crashes?), which doesn't happen in the > current state of the patch.How is this not a problem for !tbaa? -Andy> All in all, if the number of metadata is what pains the most, this > is an improvement. Otherwise, I see it more fragile, and it's also uncertain if > it's possible to implement it. > > IMHO, the best alternative presented so far is to have both metadata types > and potentially exploit the mem.parallel_loop_access MD even if there were > non-annotated instructions in the loop to *help* the dependency analysis. > isParallel() would stay as is now in the patch, it can be used as a short cut > check for a loop of which accesses all have the metadata (to skip the whole > dependency checking). > > -- > --Pekka > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu llvm.cs.uiuc.edu > lists.cs.uiuc.edu/mailman/listinfo/llvmdev
On 02/09/2013 12:48 AM, Andrew Trick wrote:> How is this not a problem for !tbaa?!tbaa is similar to the !llvm.mem.parallel_loop_access in the proposed patch. The metadata !tbaa is attached to the memory instructions which points to the "type metadata". In the proposed MD, it points to the loop id metadata. This MD gets copied automatically when you copy the instructions, e.g., in inlining or unrolling. If I understood Sebastian Pop correctly (did I?), he proposed to refer *to* the mem instructions from the llvm.loop.parallel metadata. I.e., create a metadata with the loop's memory instructions as argument values (if that's possible). This way avoiding the need for llvm.mem.parallel_loop_access MD. This MD doesn't get copied automatically as you have a MD which points *to* the original mem instructions which the newly copied instructions know nothing about. When we copy the loop branch with the loop.parallel metadata, the copied metadata still refers to the old mem instructions. -- --Pekka
Sergei Larin
2013-Feb-11 21:03 UTC
[LLVMdev] Preferential treatment of labels in MI sched DAG construction
Hi Andy, I have to resurrect an ancient question regarding scheduling boundaries. You might remember the reason for introduction of CanHandleTerminators to ScheduleDAGInstrs. In short, Hexagon is currently uses DAG construction method (buildSchedGraph) for several purposes, which includes region formation for general VLIW packetization/bundling. As such we need to handle pretty much all instructions and any terminators, including labels (though I know it sounds strange). Nevertheless, ScheduleDAGInstrs::buildSchedGraph still has this hard assert in it: assert((!MI->isTerminator() || CanHandleTerminators) && !MI->isLabel() && "Cannot schedule terminators or labels!"); ...and we have been OK till now by simply disabling it in our local version of the code, but since we are now working towards upstreaming all our code, this could no longer be ignored. I guess my proposal is not to treat labels differently from any other terminator - if a target wants to "schedule" labels - let it do so. It actually works just fine for Hexagon... so if we can change this assert to something like: assert((!MI->isTerminator() || CanHandleTerminators) && "Cannot schedule terminators or labels!"); It would equal label in rights with any other terminator. What do you think? Sergei --- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Andrew Trick
2013-Feb-11 21:22 UTC
[LLVMdev] Preferential treatment of labels in MI sched DAG construction
On Feb 11, 2013, at 1:03 PM, Sergei Larin <slarin at codeaurora.org> wrote:> Hi Andy, > > I have to resurrect an ancient question regarding scheduling boundaries. > > You might remember the reason for introduction of CanHandleTerminators to > ScheduleDAGInstrs. In short, Hexagon is currently uses DAG construction > method (buildSchedGraph) for several purposes, which includes region > formation for general VLIW packetization/bundling. As such we need to handle > pretty much all instructions and any terminators, including labels (though I > know it sounds strange). Nevertheless, ScheduleDAGInstrs::buildSchedGraph > still has this hard assert in it: > > assert((!MI->isTerminator() || CanHandleTerminators) && !MI->isLabel() && > "Cannot schedule terminators or labels!"); > > ...and we have been OK till now by simply disabling it in our local version > of the code, but since we are now working towards upstreaming all our code, > this could no longer be ignored. > > I guess my proposal is not to treat labels differently from any other > terminator - if a target wants to "schedule" labels - let it do so. It > actually works just fine for Hexagon... so if we can change this assert to > something like: > > > assert((!MI->isTerminator() || CanHandleTerminators) && "Cannot schedule > terminators or labels!"); > > It would equal label in rights with any other terminator. What do you > think?Sure, as long as CanHandleTerminators is commented to that effect. -Andy
Possibly Parallel Threads
- [LLVMdev] Preferential treatment of labels in MI sched DAG construction
- [LLVMdev] Parallel Loop Metadata
- [LLVMdev] VLIWPacketizerList: failing to schedule terminators
- [LLVMdev] VLIWPacketizerList: failing to schedule terminators
- [LLVMdev] VLIWPacketizerList: failing to schedule terminators