Daniel Berlin
2015-Jul-15 20:36 UTC
[LLVMdev] 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)
Lawrence
2015-Jul-16 04:36 UTC
[LLVMdev] 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)
Lawrence
2015-Jul-17 00:43 UTC
[LLVMdev] Register pressure mechanism in PRE or Smarter rematerialization/split/spiller/coalescing ?
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)
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) >
Xinliang David Li
2015-Jul-17 17:45 UTC
[LLVMdev] Register pressure mechanism in PRE or Smarter rematerialization/split/spiller/coalescing ?
It would be helpful if you can file a bug with a test case. David 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 >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20150717/48605012/attachment.html>