[polly] r287270 - [FIX] Do not try to hoist memory intrinsic

[zino via llvm-commits]

We will launch a new run and  new bugs :)

-Zino

-----Original Message-----
From: Tobias Grosser [mailto:tobias at grosser.es] 
Sent: Friday, November 18, 2016 8:10 AM
To: llvm-commits at lists.llvm.org
Cc: efriedma at codeaurora.org; Johannes Doerfert <doerfert at cs.uni-saarland.de>
Subject: Re: [polly] r287270 - [FIX] Do not try to hoist memory intrinsic



On Fri, Nov 18, 2016, at 01:47 PM, Johannes Doerfert via llvm-commits
wrote:
> On 11/18, Tobias Grosser wrote:
> > On Thu, Nov 17, 2016, at 11:11 PM, Johannes Doerfert via 
> > llvm-commits
> > wrote:
> > > Author: jdoerfert
> > > Date: Thu Nov 17 16:11:56 2016
> > > New Revision: 287270
> > > 
> > > URL: http://llvm.org/viewvc/llvm-project?rev=287270&view=rev
> > > Log:
> > > [FIX] Do not try to hoist memory intrinsic
> > > 
> > > Since we do not necessarily treat memory intrinsics as non-affine 
> > > anymore, we have to check for them explicitly before we try to 
> > > hoist an access.
> > 
> > Hi Johannes,
> > 
> > thanks for the fix! Would it be possible to add a test case or is 
> > this hard for some reason?
> I did not have time to distill one from the SPEC benchmarks (and 
> possibly others) that failed (with enabled hoisting) after the "memory 
> intrinsics are affine patch". However, it should not be hard though. 
> Any affine memory read that was caused by a memory intrinsic, e.g., 
> memcpy, should do...
> 
> I would guess that is sufficient to explicitly enabling hoisting for 
> the memcpy testcase we already have to exposed the problem. Since 
> initially there was no flag to disable hoisting, no test case 
> explicitly enables it, thus the test coverage we lost (unit tests + 
> lnt) by disabling hoisting is enormous...

No worries if you are busy. This can wait a couple of days.
 
> In my opinion the actual coverage problem is that hoisting is disabled 
> globally on __all__ buildbots. Using __only__ profitable buildbots for 
> a while caused us so much pain at some point. I would not have thought 
> we would willingly go through the same trouble again but it seems 
> history repeats itself.

Right. We really should not have had to turn this off, but unfortunately this mode caused bugs in LNT, ffmpeg and -- as Zino told me on the LLVM dev-mtg -- also on Android, which neither you nor me could commit to fix at this time.

Now, I would be more than glad to turn this on again as fast as possible. It seems we are already LNT clean. I am happy to check ffmpeg.

@Eli/Zino, do you at Qualcomm still see bugs/issues related to load hoisting, which we should be aware of?

Best,
Tobias