On Sun, Jul 7, 2019 at 9:48 PM Akshat Garg <xks...@gmail.com> wrote: > On Sun, Jul 7, 2019 at 7:49 PM Paul E. McKenney <paul...@linux.ibm.com> > wrote: > >> On Sat, Jul 06, 2019 at 12:39:45PM +0530, Akshat Garg wrote: >> > On Sat, Jul 6, 2019 at 1:09 AM Akshat Garg <xks...@gmail.com> wrote: >> > >> > > On Fri, Jul 5, 2019 at 7:18 AM Paul E. McKenney < >> paul...@linux.ibm.com> >> > > wrote: >> > > >> > >> [CCing Ramana] >> > >> >> > >> My guess is that Jakub is saying that you should start by >> > >> replacing all instances of TREE_VOLATILE() with (TREE_VOLATILE() >> > >> || TREE_DEPENDENT_PTR()), give or take my getting the names wrong. >> > >> Then selectively remove instances of TREE_DEPENDENT_PTR() where it >> can >> > >> be shown to be safe to do so. >> > >> >> > > >> > >> The advantage of Jakub's approach over selectively inserting >> > >> TREE_DEPENDENT_PTR() where it is needed is less chance of bugs due >> > >> to missing TREE_DEPENDENT_PTR() instances. >> > >> >> > > I have checked that replacing TREE_VOLATILE() with (TREE_VOLATILE() || >> > > TREE_DEPENDENT_PTR()) is not sufficient to make the dependent ptr act >> > > similar to volatile ptrs. I am looking over the source and will let >> you >> > > know if I am able to do it. If any of like, I can share the patch. >> > > >> > > Akshat >> > > >> > I have made some changes to the previous commit for _Dependent_ptr >> > qualifier. We need to use this ( >> > >> https://github.com/AKG001/gcc/blob/fb4187bc3872a50880159232cf336f0a03505fa8/gcc/tree-core.h#L943 >> ) >> > side-effect flag also with the dependent_ptr flag therefore, I have >> pulled >> > out the dependent_ptr from the union. Also, I have introduced a new >> macro >> > for dependent ptr TREE_THIS_DEPENDENT_PTR. Similar macro >> > (TREE_THIS_VOLATILE) is used to check whether anything is volatile or >> not. >> > The initial plan was to change all occurrences of checks for volatile >> with >> > volatile or dependent_ptr, i.e., TREE_THIS_VOLATILE(NODE) with >> > (TREE_THIS_DEPENDENT_PTR(NODE) || TREE_THIS_VOLATILE(NODE)). But, I >> found >> > 250 places to do this. Would that be okay, if I still change over all >> the >> > places? >> >> There is only one way to find out! And in any case, this sounds like >> one reason why your previous changes didn't fully mimic the volatile >> keyword. ;-) >> >> So I recommend making the changes. >> >> Probably not quite large enough a change to make it worthwhile learning >> Coccinelle, but that is nevertheless an option. (This tool allows you >> to specify update patterns in C code, which it will automatically apply.) >> http://coccinelle.lip6.fr/ if you want to try it out. >> >> Thanx, Paul > > I have made the changes shown here https://github.com/AKG001/gcc/commit/e4ffd77f62ace986c124a70b90a662c083c570ba The changes are not much tested on the edge cases. The test it was breaking earlier shows here https://github.com/AKG001/test/blob/master/dependency_breaking.c I am checking for other test cases. The optimized code with -O3 after applying the patch is here https://github.com/AKG001/test/blob/master/dependency_breaking.c.231t.optimized Please, I need your opinion and further directions on this.
Thanks, Akshat > > >> Jakub also seems to be pointing out that optimizations in the backend > > >> also need to be handled, which I believe is what he is getting at > with his > > >> mention of RTL. Of course, there is a very large number of backends, > so > > >> there needs to be a way of only doing some of them. One way of doing > that > > >> is to handle only the multicore weakly ordered CPUs (ARM, ARMv8, > PowerPC, > > >> MIPS, maybe RISC-V), and on other CPUs continue the current behavior > > >> of transforming the memory_order_consume load to memory_order_acquire. > > >> For example, memory_order_acquire is almost free on x86, s390, and > > >> the like. > > >> > > >> Does that help? > > >> > > >> Thanx, Paul > > >> > > >> On Fri, Jul 05, 2019 at 04:38:03AM +0530, Akshat Garg wrote: > > >> > On Thu, Jul 4, 2019 at 11:39 PM Jakub Jelinek <ja...@redhat.com> > wrote: > > >> > > > >> > > On Thu, Jul 04, 2019 at 10:40:15AM -0700, Paul E. McKenney wrote: > > >> > > > > I think fully guaranteeing this is hard (besides when you use > > >> > > > > volatile), we have the very same issue when dealing with > > >> > > > > pointer provenance rules, known for years and not fixed > > >> > > > > (and I don't see a good way to fix these issues without > > >> > > > > sacrifying performance everywhere). > > >> > > > > > > >> > > > > Good luck ;) > > >> > > > > > >> > > > Thank you, we will need it! ;-) > > >> > > > > >> > > Well, luck probably isn't all you need, you'll need some way to > > >> represent > > >> > > those dependencies in the IL (both GIMPLE and RTL) that would > ensure > > >> that > > >> > > they aren't optimized away, because just hoping that optimization > > >> don't > > >> > > mess > > >> > > that up or just patching a few optimizations you discover first > isn't > > >> going > > >> > > to be very reliable. Say don't rewrite those into SSA form and > > >> represent > > >> > > them close to how volatile ptrs are handled at least for the > > >> beginning, and > > >> > > if there are safe optimizations special case those, rather than > > >> allowing > > >> > > all > > >> > > optimizations on those and hope it will work out. > > >> > > > > >> > > Jakub > > >> > > > > >> > I don't understand this statement completely "Say don't rewrite > those > > >> into > > >> > SSA form and represent them close to how volatile ptrs are handled > at > > >> least > > >> > for the beginning, and if there are safe optimizations special case > > >> those, > > >> > rather than allowing all optimizations on those and hope it will > work > > >> > out.". > > >> > Are you saying that we should try to represent dependent ptrs as > > >> volatile > > >> > ptrs for some places? We should figure out all the places where a > check > > >> > for volatile ptrs happens and we check for dependent ptrs there > also and > > >> > see if we can allow the optimization or not. > > >> > Am I getting you correctly, please tell? > > >> > > > >> > -Akshat > > >> > > >> >
