On 16 October 2014 09:34, Hal Finkel <hfinkel at anl.gov> wrote:> Interesting. Looks like the problem is in (219545, 219569].Yes.> and given the magnitude of the change, I think that the trip-count changes are more likely.Good point.> Of course, all of these things are bug fixes :( -- So how do we follow-up on this?Correctness before performance. Always. I'll create a bug on this pointing to the delta for someone to investigate. It doesn't have to be me, or the committer, the idea is that this is not high priority. At least, not for now. Once we have a way to track this more consistently, I think a good approach is to be pragmatic. So, something along the lines of working with the implementer, trying to understand the reason of the regression. It could be a bad implementation or just a target-specific reason for the regression. Depending on the importance of the regression and of the patch, I'd consider turning it off for ARM (for example, PR18996) and later investigating why. I'd only consider reverting the patch in extreme circumstances, for example, if we're close to a release AND the regression is big AND the patch is a new feature, etc. I believe that's what you were concerned about. :) cheers, --renato
> On Oct 16, 2014, at 3:04 AM, Renato Golin <renato.golin at linaro.org> wrote: > > On 16 October 2014 09:34, Hal Finkel <hfinkel at anl.gov> wrote: >> Interesting. Looks like the problem is in (219545, 219569]. > > Yes.Chandler’s complex arithmetic changes are also in the range: r219557 in clang. We saw it change the code in mandel-2 significantly. Adam>> and given the magnitude of the change, I think that the trip-count changes are more likely. > > Good point. > > >> Of course, all of these things are bug fixes :( -- So how do we follow-up on this? > > Correctness before performance. Always. > > I'll create a bug on this pointing to the delta for someone to > investigate. It doesn't have to be me, or the committer, the idea is > that this is not high priority. At least, not for now. > > Once we have a way to track this more consistently, I think a good > approach is to be pragmatic. So, something along the lines of working > with the implementer, trying to understand the reason of the > regression. It could be a bad implementation or just a target-specific > reason for the regression. Depending on the importance of the > regression and of the patch, I'd consider turning it off for ARM (for > example, PR18996) and later investigating why. > > I'd only consider reverting the patch in extreme circumstances, for > example, if we're close to a release AND the regression is big AND the > patch is a new feature, etc. I believe that's what you were concerned > about. :) > > cheers, > --renato > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
On Thu, Oct 16, 2014 at 9:56 PM, Adam Nemet <anemet at apple.com> wrote:> > On Oct 16, 2014, at 3:04 AM, Renato Golin <renato.golin at linaro.org> > wrote: > > > > On 16 October 2014 09:34, Hal Finkel <hfinkel at anl.gov> wrote: > >> Interesting. Looks like the problem is in (219545, 219569]. > > > > Yes. > > Chandler’s complex arithmetic changes are also in the range: r219557 in > clang. We saw it change the code in mandel-2 significantly.I would not at all be surprised by regression in some cases here. Complex arithmetic with reals got *much* faster. Complex arithmetic with other complex numbers may have gotten slower. If there were fastmath flags, *crazy preposterously slower*. Note that I have a patch out needing review which addresses most of the regression in this case, and should address all of the regressions if fastmath flags are in play. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20141016/3dceba98/attachment.html>
> Chandler’s complex arithmetic changes are also in the range: r219557 in clang. We saw it change the code in mandel-2 significantly.mandel-2 is broken on hard FP ABI systems, btw. The reason is simply: we're emitting a call to __muldc3 with AAPCS VFP calling convention, however, the function expects softfp (AAPCS) calling conv and reads garbage from GP registers. I'm working on fix. -- With best regards, Anton Korobeynikov Faculty of Mathematics and Mechanics, Saint Petersburg State University