Hi Derek, On Wed, 16 Dec 2015 17:16:26 +0000, Derek Buitenhuis wrote: > On 12/15/2015 10:39 PM, Jean Delvare wrote: > > I see two completely different things. > > > > The change I proposed is specific to one function, only changes const > > vs non-const, and the object under discussion is being accessed for > > reading only (thus the const pointer.) It would remove a memcpy but > > does not introduce a direct copy (with =.) > > See below. > > > The change you pointed us to is touching very generic functions, > > handling by nature a very wide variety of pointer types for both > > reading and writing. > > > > So just because the change you pointed us to may be good and desirable, > > doesn't imply that the change I am proposing is bad and undesirable. > > > > Side note #1: if gcc really considers pointer types that only differ by > > "const" as different when it comes to code optimization, it is > > seriously broken IMHO. Unfortunately the man page is quite vague on the > > topic ("unless the types are almost the same", can you be more vague > > please.) > > The C spec does. GCC happens to handle it correctly. We support a whole lot > more compilers than GCC.
The C spec does say what pointers types are considered the same and what pointer types should be considered different. Compilers can use this to warn users about pointer mismatches. That's what -Wstrict-aliasing is about. But does the C spec say that compilers should use pointer type differences to optimize the code by reordering the instructions? I doubt it. This very much looks like an implementation decision on the compiler side. That's what -fstrict-aliasing is about. Obviously both are related as far as the compiler is concerned, as the pointer type tracking and comparison code has to be common. These are two different things nevertheless. > > Side note #2: if strict-aliasing optimizations can do so much damage > > to your code that you end up doing > > memcpy(arg, &(void *){ NULL }, sizeof(val)); > > to set a pointer to NULL, maybe you should consider building with > > -fno-strict-aliasing. > > Again, that is GCC-specific. My point is that if we *can* be correct, with > regards to the spec, we should be. We don't gain anything, IMO, by removing > this particular thing. Performance, readability, consistency, is what we gain. Apparently you think they are not worth it, and while I'm surprised, I respect your decision. > > Side note #3: I'm surprised this change was needed in the first place > > as my understanding was that gcc would skip all strict-aliasing > > optimizations for void pointers, which is what the affected functions > > are handling. It's sad that the commit message is as vague as gcc's > > manual page on the topic. > > If a change makes some code more spec compliant, with little to no downside, > I fail to see why it shouldn't be incldued. > > Your patch here makes code *less* spec compliant for little to no gain. My code is doing the same that is already done everywhere in the ffmpeg source code tree. It is (maybe) less spec compliant if you look only at the function I modified. If you look at the project as a whole, it doesn't make any difference. If such casts were already a problem, then whoever is affected can already not use ffmpeg, with or without my patch. Is it the case? Is there any platform where ffmpeg doesn't work because of this? Any compiler that can't be used to build ffmpeg because of this? And this is why I'm surprised, really. There are only two positions which I can understand. Either these casts are bad, they should be removed from ffmpeg immediately, and no new occurrence should be accepted. Or they are not bad, and there should be no objection introducing new ones. Anything in the middle (as is happening right now) makes no sense to me. My own understanding of the situation is that such casts can be problematic in certain cases [1] and this is why the compilers warn about them. But the C spec allows for explicit cast between pointer types for a reason, and that reason is that they are sometimes the right thing to do. Which I think is the case here. [1] http://c-faq.com/ansi/constmismatch.html > P.S. It actually looks like the original code is not entirely compliant, after > discussing with some people Smarter Than Me: > > [17:06] <+courmisch> memory copying the pointer fixes the aliasing problem on > pointer itself > [17:06] <+courmisch> but I suspect it leaves an aliasing problem with the > pointed data > [17:07] <+courmisch> specifically, I am not sure the standard allows creating > pointers to existing data out of thin air You do exactly that as soon as you pass a pointer as a parameter to a function. So the standard better allows it. The problem isn't with creating new pointers to existing data, it is creating new pointers of a different type than the original pointers had. > [17:07] <+courmisch> which memcpy() effectively does if it changes the > pointer type > > So, as a I see it, this patch replaced one aliasing violation with two. I'm not sure why two, and even if that is the case, I'm not sure what difference it makes. If both approaches break the standard, I'd go for the faster or cleaner one, not the one breaking the standard the fewest times. > If an argument can be made that there is a measurable speedup, and none of > our supported > compilers abuse the C spec, as compilers are wont to do, then perhaps it's > worth revisiting, > but otherwise, I'd prefer to NAK it. I'm obviously not The Final Word here, > but this is > my 2 cents. I'm honestly puzzled. Not to get one of my patches refused, that's part of the process, it's not the first time and I'm fine with that. But the whole thing seems to escape logic, twice. Firstly, the point of -fstrict-aliasing is to optimize the code. But as it is dangerous, you are advocating that pointers should never be cast and data should always be copied to temporary buffers instead. The optimization gain from -fstrict-aliasing would have to be significant to justify adding extra data copy everywhere. I have a serious doubt that merely reordering a few instructions can bring in a performance gain that the extra copies wouldn't at least cancel. Secondly, the debate here is around const pointers. My original understanding was that the point of marking function parameters as const was so that the compiler could warn us if a function was modifying pointed data that is was only supposed to read. If in fact this is only a side effect and actually "const" means a completely different pointer type to the compiler and it uses that for reordering instructions for optimization purposes, then I'm afraid it is being misused pretty much everywhere. And if you introduce temporary buffers to get rid of such warnings, I would question the value of using "const" in your function parameters in the first place. You'd probably be better just dropping these const markers so that you can keep your code clean and fast. -- Jean Delvare SUSE L3 Support _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel