[PATCH] D18830: [llvm] GVN.cpp: Do not swap when both LHS and RHS are arguments.

[Aditya K via llvm-commits]

> "if we do, we should fix that"": isn't it what the code actually does a 
> few lines below? 
Few lines below the swap happens based on the Value Number, so that is unrelated to my patch.
The patch will avoid a redundant swap operation when both LHS and RHS are arguments. In some cases 
it might prevent two swaps from happening. e.g., when LHS and RHS both are arguments and GVN(LHS)> GVN(RHS)

-Aditya

> 
> I don't mind at all this patch going in, the motivation for it is just 
> unclear to me (it is NFC right?). 


________________________________
> Subject: Re: [PATCH] D18830: [llvm] GVN.cpp: Do not swap when both LHS 
> and RHS are arguments. 
> From: mehdi.amini at apple.com 
> Date: Wed, 27 Apr 2016 16:28:41 -0700 
> CC: reviews+D18830+public+bbad7dc1e79098a7 at reviews.llvm.org; 
> hiraditya at msn.com; sebpop at gmail.com; mcrosier at codeaurora.org; 
> chandlerc at gmail.com; llvm-commits at lists.llvm.org 
> To: dberlin at dberlin.org 
> 
> 
> On Apr 7, 2016, at 8:17 AM, Daniel Berlin 
> <dberlin at dberlin.org<mailto:dberlin at dberlin.org>> wrote: 
> 
> I think his point is more the same one i made: "we shouldn't have to 
> swap at all, and if we do, we should fix that" 
> 
> "if we do, we should fix that"": isn't it what the code actually does a 
> few lines below? 
> 
> I don't mind at all this patch going in, the motivation for it is just 
> unclear to me (it is NFC right?). 
> 
> -- 
> Mehdi 
> 
> 
> 
> On Thu, Apr 7, 2016 at 7:46 AM, Aditya Kumar 
> <hiraditya at msn.com<mailto:hiraditya at msn.com>> wrote: 
> hiraditya added a comment. 
> 
> In http://reviews.llvm.org/D18830#394173, @chandlerc wrote: 
> 
>> Please don't avoid extra work in this way IMO, unless you have a very 
> significant compile time hit due to this. 
>> 
>> We have a pervasive reliance on canonicalizing constants to the RHS 
> of binary operations so those should be exceedingly rare anyways. We 
> shouldn't fight that, we should run with it. 
>> 
>> My two cents. If Danny, Chad, or others working more on GVN disagree, 
> they can happily override me. =D 
> 
> 
> Thanks for the feedback. This patch will not change the order of 
> preference (RHS should keep Constants/Arguments). it only prevents swap 
> when both LHS and RHS are Arguments. 
> 
> 
> Repository: 
> rL LLVM 
> 
> http://reviews.llvm.org/D18830 
> 
> 
> 
> 
>