[PATCH] D17652: [CGP] Duplicate addressing computation in cold paths if required to sink addressing mode

[Mehdi Amini via llvm-commits]

Hi Philip,

> On Mar 4, 2016, at 2:36 PM, Philip Reames <listmail at philipreames.com> wrote:
> 
> 
> 
> On 03/04/2016 02:31 PM, Mehdi Amini wrote:
>>> On Mar 4, 2016, at 2:29 PM, Philip Reames <listmail at philipreames.com> wrote:
>>> 
>>> reames added inline comments.
>>> 
>>> ================
>>> Comment at: test/Transforms/CodeGenPrepare/X86/sink-addrmode.ll:10
>>> @@ +9,3 @@
>>> +; CHECK-LABEL: @test1
>>> +; CHECK: add i64 %sunkaddr, 40
>>> +entry:
>>> ----------------
>>> joker.eph wrote:
>>>> (I know you didn't create, but) having named value should be avoided for optimized build, we should fix CGP. Adding tests that relies on the named value is making it harder to fix this.
>>> Huh?  This is the first I've heard of this advice.  Named values are widespread in the optimizer.
>> Any optimization that is using the IRBuilder won't introduce named value in optimized build.
> Please point to something in the style guide or reference where this has been previously discussed?

I can be wrong, I was just repeating what I was told. I think Duncan and/or Chandler mentioned this to me. I was *assuming* we had such a policy, but your reaction seems to prove me wrong.

A couple of months ago, I saw many calls to strlen() on my profile a couple of months ago, and I was told that it was a known issue that we need to fix when passes are not using the IRBuilder.
Since I had this on my "TODO" list for some time now, I just noticed that your test relies on the named value, and realized at the same time that it's gonna be a lot of work I didn't expect to fix all the tests...

Here is how I see it:

- We have a such a "policy" and we are seeking to actively eliminate named value introduced by the optimizer in release build. In such case I was regretting that this test is making it harder for whoever wants to "fix" CGP.
- We don't have a policy, then I apologize and please move on and commit your patch. We can have a separate discussion about if such a policy is desirable and how we're gonna work with it.

I hope my approach sounds reasonable to you?

Best,

-- 
Mehdi