On 08/28/2017 06:20 PM, Martin Sebor wrote: >> >> Warning for memcpy (p, p, ...) is going to fire false positives all >> around >> given the C++ FE emits those in some cases and optimization can >> expose that we are dealing with self-assignments. And *p = *p is >> valid. > > I changed it to only warn for calls to the library function and > not to the __builtin_xxx. Though I haven't been able to reproduce > the problem you referring to (bug 32667) which makes me wonder if > it's been fixed. I doubt it it's been fully addressed. I don't know if it's possible, but if we could somehow flag the assignments as they're generated by the front-ends for the structure copy, that would probably be best becuase we could then ignore those, but detect on all the others.
> >> >> @@ -1028,6 +1066,10 @@ gimple_fold_builtin_memory_op >> (gimple_stmt_iterator *gsi, >> } >> } >> >> + /* Avoid folding the call if overlap is detected. */ >> + if (check_overlap && detect_overlap (loc, stmt, dest, src, len)) >> + return false; >> + >> >> no, please not. You diagnosed the call (which might be a false positive) >> so why keep it undefined? The folded stmt will either have the same >> semantics (aggregate copies expanded as memcpy) or have all reads >> performed before writes. > > My goal was to follow the approach reflected in the comments > elsewhere in the file: > > /* Out of bound array access. Value is undefined, > but don't fold. */ So I misunderstood a bit from our call today. As long as we have the same semantics I don't have a real strong opinion about folding here. There are cases where folding is going to enable further optimizations and cases where it's likely to inhibit optimization. A lot depends on the exact context of the nearby code, the operands (particularly the size). I doubt there's any consensus to be had on this topic because the implications of folding at point likely aren't very predictable. My personal opinion is we should only be folding this stuff early if we collapse down to 1 or 2 stores. Those are the cases that are most likely going to result in further optimization. The rest can probably be deferred with marginal, if any, performance penalty. > While gimple_fold_builtin_memory_op may be able to provide well- > defined behavior for the otherwise undefined semantics in a small > subset of cases, it doesn't attempt to fold many more that it > otherwise could (it only folds calls with sizes that are powers > of 2). So it seems of dubious value to make an effort in this > relatively small subset of cases. I agree its of dubious value. Again, no strong opinion from me, I'll go with a majority opinion here. > > In my experience, users also don't appreciate optimizations that > "rely on" undefined behavior one way or the other. What they would > like to see instead is that when their compiler detects undefined > behavior it diagnoses it but either doesn't use it to make > optimization decisions, or uses it to disable them. For calls to > library functions, that in my view means making the call and not > folding it. (Btw., do we have some sort of a policy or guideline > for how to handle such cases in general?) It's never a fine crisp line. There's almost always two sides to this kind of question and they simply will never agree because they have different priorities. So we end up having to make a bit of a guess about how often the particular undefined situation occurs in practice, what the implications of optimizing around it might be, etc. It's almost always the case that some people complain, but that in and of itself is not enough to justify never optimizing these cases. It's a judgment call. Jeff
