Daniel Berlin
2015-Jul-17 01:04 UTC
[LLVMdev] Register pressure mechanism in PRE or Smarter rematerialization/split/spiller/coalescing ?
That should be literally impossible, which makes me think something was tested wrong The second patch i posted disables scalar pre (assuming you use -disable-pre) but not load pre. Since the patch you reverted touched only scalar pre, disabling scalar pre should *also* do the same thing. On Thu, Jul 16, 2015 at 5:43 PM, Lawrence <lawrence at codeaurora.org> wrote:> Hi, Daniel: > > Something interesting, even though your patch performed better for our precheckin perf run, it doesn't help the test I was looking at, for my case, revert your patch give the best results. > > I will revert internally for now, and looking for better solution in the future. > > Regards > > Lawrence Hu > > > -----Original Message----- > From: Lawrence [mailto:lawrence at codeaurora.org] > Sent: Wednesday, July 15, 2015 9:36 PM > To: 'Daniel Berlin' > Cc: 'LLVM Developers Mailing List' > Subject: RE: Register pressure mechanism in PRE or Smarter rematerialization/split/spiller/coalescing ? > > Hi, Daniel: > > Thanks, I tried that patch you provided, it is better than just disabling your previous patch, it has more improvements than degradations. > > Do you want to post that patch or you want me to do that? > > Regards > > Lawrence Hu > > -----Original Message----- > From: Daniel Berlin [mailto:dberlin at dberlin.org] > Sent: Wednesday, July 15, 2015 1:36 PM > To: Lawrence > Cc: LLVM Developers Mailing List > Subject: Re: Register pressure mechanism in PRE or Smarter rematerialization/split/spiller/coalescing ? > > On Wed, Jul 15, 2015 at 1:10 PM, Daniel Berlin <dberlin at dberlin.org> wrote: >> IMHO, This doesn't make a lot of sense to turn off this part on it's own. >> I would just use the enable-pre flag to turn off scalar PRE, as it >> will cause the same issue in other cases as well. >> Is there some reason you aren't just doing that? >> I suspect if this is a performance win, that would be as well. >> > > Ugh, actually, it should be a win with the following change: > > > diff --git a/lib/Transforms/Scalar/GVN.cpp b/lib/Transforms/Scalar/GVN.cpp index 2c47a8a..a3387e3 100644 > --- a/lib/Transforms/Scalar/GVN.cpp > +++ b/lib/Transforms/Scalar/GVN.cpp > @@ -1767,7 +1767,7 @@ bool GVN::processNonLocalLoad(LoadInst *LI) { > } > > // Step 4: Eliminate partial redundancy. > - if (!EnablePRE || !EnableLoadPRE) > + if (!EnableLoadPRE) > return false; > > return PerformLoadPRE(LI, ValuesPerBlock, UnavailableBlocks); > > > > > This will disable Scalar PRE without disabling load PRE. > > > (note, again, however, that load PRE can create exactly the same GEP situation you are referring to) >
Philip Reames
2015-Jul-17 17:07 UTC
[LLVMdev] Register pressure mechanism in PRE or Smarter rematerialization/split/spiller/coalescing ?
On 07/16/2015 06:04 PM, Daniel Berlin wrote:> That should be literally impossible, which makes me think something > was tested wrong > > The second patch i posted disables scalar pre (assuming you use > -disable-pre) but not load pre. > > Since the patch you reverted touched only scalar pre, disabling scalar > pre should *also* do the same thing.Unless that benchmark is relying on *some* scalar PRE happening, but not "too much". i.e. it could be a really really fragile benchmark which seems to fit the description.> > > On Thu, Jul 16, 2015 at 5:43 PM, Lawrence <lawrence at codeaurora.org> wrote: >> Hi, Daniel: >> >> Something interesting, even though your patch performed better for our precheckin perf run, it doesn't help the test I was looking at, for my case, revert your patch give the best results. >> >> I will revert internally for now, and looking for better solution in the future. >> >> Regards >> >> Lawrence Hu >> >> >> -----Original Message----- >> From: Lawrence [mailto:lawrence at codeaurora.org] >> Sent: Wednesday, July 15, 2015 9:36 PM >> To: 'Daniel Berlin' >> Cc: 'LLVM Developers Mailing List' >> Subject: RE: Register pressure mechanism in PRE or Smarter rematerialization/split/spiller/coalescing ? >> >> Hi, Daniel: >> >> Thanks, I tried that patch you provided, it is better than just disabling your previous patch, it has more improvements than degradations. >> >> Do you want to post that patch or you want me to do that? >> >> Regards >> >> Lawrence Hu >> >> -----Original Message----- >> From: Daniel Berlin [mailto:dberlin at dberlin.org] >> Sent: Wednesday, July 15, 2015 1:36 PM >> To: Lawrence >> Cc: LLVM Developers Mailing List >> Subject: Re: Register pressure mechanism in PRE or Smarter rematerialization/split/spiller/coalescing ? >> >> On Wed, Jul 15, 2015 at 1:10 PM, Daniel Berlin <dberlin at dberlin.org> wrote: >>> IMHO, This doesn't make a lot of sense to turn off this part on it's own. >>> I would just use the enable-pre flag to turn off scalar PRE, as it >>> will cause the same issue in other cases as well. >>> Is there some reason you aren't just doing that? >>> I suspect if this is a performance win, that would be as well. >>> >> Ugh, actually, it should be a win with the following change: >> >> >> diff --git a/lib/Transforms/Scalar/GVN.cpp b/lib/Transforms/Scalar/GVN.cpp index 2c47a8a..a3387e3 100644 >> --- a/lib/Transforms/Scalar/GVN.cpp >> +++ b/lib/Transforms/Scalar/GVN.cpp >> @@ -1767,7 +1767,7 @@ bool GVN::processNonLocalLoad(LoadInst *LI) { >> } >> >> // Step 4: Eliminate partial redundancy. >> - if (!EnablePRE || !EnableLoadPRE) >> + if (!EnableLoadPRE) >> return false; >> >> return PerformLoadPRE(LI, ValuesPerBlock, UnavailableBlocks); >> >> >> >> >> This will disable Scalar PRE without disabling load PRE. >> >> >> (note, again, however, that load PRE can create exactly the same GEP situation you are referring to) >> > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Daniel Berlin
2015-Jul-17 17:18 UTC
[LLVMdev] Register pressure mechanism in PRE or Smarter rematerialization/split/spiller/coalescing ?
On Fri, Jul 17, 2015 at 10:07 AM, Philip Reames <listmail at philipreames.com> wrote:> On 07/16/2015 06:04 PM, Daniel Berlin wrote: >> >> That should be literally impossible, which makes me think something >> was tested wrong >> >> The second patch i posted disables scalar pre (assuming you use >> -disable-pre) but not load pre. >> >> Since the patch you reverted touched only scalar pre, disabling scalar >> pre should *also* do the same thing. > > Unless that benchmark is relying on *some* scalar PRE happening, but not > "too much". i.e. it could be a really really fragile benchmark which seems > to fit the description.Sorry, yes, let me rephrase: It should not be having performance loss due to the same root cause. The failure mode described was GEP live ranges being extended due to new phis that PRE was inserting. That should not be happening, because it can't be doing that anymore :)