llvm dev - Jul 2015 - [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)

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)

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)

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)
>

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>