[llvm-commits] [llvm] r168181 - in /llvm/trunk: lib/Transforms/InstCombine/InstCombineCompares.cpp test/Transforms/InstCombine/icmp.ll

[Duncan Sands]

Hi Takumi,

On 25/11/12 00:52, NAKAMURA Takumi wrote:
>>>>   I do no think merging partial patches into the release branch
>>>> should be the way we deal with issues unless there is no other
>>>> way the to fix a problem.
>>>
>>> I proposed minimal fix. r168196 is "similar simplification without any
>>> dependence each other."
>>> To be more consistent, I might suggest "both r168186 and r168196".
>>> They could be applied cleanly on the release_32.
>>
>>
>> Sounds good to me, however the release process requires approval from
>> the code owner so your first point of contact should be the code
>> owner or owners of the r168186 and r168196.
>
> Duncan,
>
> I propose as below, two options;
>
> 1) r168186 and r168196

I think this is the best option, as the first of these does fix a real bug
(though it's hard to get a miscompile out of it, it is theoretically possible).
The corresponding instcombine patch was already approved by Chris and applied;
this is exactly the same fix but for duplicated code in instsimplify.  The
second patch improves the comments and (more importantly) gets rid of a warning
when building with clang.  It is zero risk, so should just be applied.

In short, I approve applying both of these.

That said, I'm not the code owner: Chris is (by default since no code owner
has been assigned yet).  Chris?

Ciao, Duncan.


>
> 2) InstCombineCompares.cpp in r168196 (partial)
>
> Could you please approve either to the release_32?
> (Or could you delegate it to other responsible person?)
> I am sure I can propose to let the tree free from warnings (with clang).
>
> ...Takumi
>