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

[Mehdi Amini via llvm-commits]

> On Mar 9, 2016, at 1:00 PM, Philip Reames <listmail at philipreames.com> wrote:
> 
> As a gesture of good faith, I've updated the patch under discussion to avoid relying on the named instructions.
> 
> On the general topic of value naming, I've replied to the review Mehdi mentioned (http://reviews.llvm.org/D17946) with my opinions on the matter.  My main concern is making sure that we don't introduce an expectation that release mode code doesn't have value names. Having the value names is invaluable from a debugging standpoint. It seems like the existing proposal in that review would not break this use case, so my concern is much less than I initially thought it might be.
> 
> Mehdi, just to confirm, are you still happy to see this patch land? The discussion got a bit fragmented so I want to confirm before landing it.

LGTM.

Side note: I explicitly approved this revision on Monday after noticing that we don't have a written policy anywhere on the named values in release build (even though the few colleagues I asked around thought we did). Most also thought (just like me) that the IRBuilder would do it automatically (while it is opt-in).

Thanks,

-- 
Mehdi


> 
> Philip
> 
> On 03/04/2016 02:51 PM, Philip Reames wrote:
>> 
>> 
>> On 03/04/2016 02:36 PM, Philip Reames 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?
>>>> 
>>>>> If we want to change that, it's definitely a separate change.
>>>> I don't ask you to change CGP to not emit a named value. I am asking the test not to rely on them.
>>> And I'm going to ignore this request since it's irrelevant to the change at hand and should be fixed in a single patch which changes the code if desired.
>> Let me retract this statement.  My phrasing and the attitude it carried wasn't professional.  Sorry.  I'm going to disengage from this until Monday and come back at it when I'm in a better headspace.
>> 
>> Philip
>