[LLVMdev] Register pressure mechanism in PRE or Smarter rematerialization/split/spiller/coalescing ?

[Daniel Berlin]

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