[llvm-commits] [llvm] r167740 - in /llvm/trunk: lib/Transforms/Scalar/Reassociate.cpp test/Transforms/Reassociate/mul_neg.ll test/Transforms/Reassociate/multistep.ll

[Shuxin Yang]

Hi, Chandler:

    I didn't take laptop with me that day, I could not verify what you 
told me during the dev meeting.
I reverify the SetVector. The set semantic is enforced by SmallSet which 
is std::set if the set is not
small any more. While I "unfortunately choose std::set", it is not worse 
than the set in SetVector.

   I remember you told me SmallVector is using hash map. Maybe in the 
old release, it is using hashmap.
I don't think hashmap works better than std::map if sizeof(set) is small 
to medium. The reason is n-fold:

   - the construction of bucket of hashmap is relatively very high if 
the sizeof(map) is quite small.
     At lease I came across this problem on HP-UX.

    - In terms of locality, std::map is better than hash_map.
    - hash_map use %prime, which is very expensive, and it is especially 
true if div operation is not fully pipilined.
      But this is weak argument, as typical compiler tends to have low ILP.
    - hash_map is system dependent.

  SetVector is clunky to use in this pass, as we need to introduce 
another container to keep track of dead instruction.
There are other reasons...


On 11/12/2012 07:17 PM, Chandler Carruth wrote:
> On Mon, Nov 12, 2012 at 7:12 PM, Evan Cheng <evan.cheng at apple.com> wrote:
>> On Nov 12, 2012, at 7:00 PM, Chandler Carruth <chandlerc at google.com> wrote:
>>
>>> On Mon, Nov 12, 2012 at 6:55 PM, Evan Cheng <evan.cheng at apple.com> wrote:
>>>> On Nov 12, 2012, at 3:51 PM, Chris Lattner <clattner at apple.com> wrote:
>>>>
>>>>> On Nov 12, 2012, at 1:08 PM, Evan Cheng <evan.cheng at apple.com> wrote:
>>>>>
>>>>>> Hi Eli,
>>>>>>
>>>>>> The approach seems sound to me. Since no one has raised design concerns at this point and the fact this is a a bug fix for a crash, I think it's appropriate for review after commit.
>>>>> This patch isn't "obvious", and Shuxin is still new to this part of the code.  Unless it's been reviewed it shouldn't go in.
>>>> The patch was submitted for review back on 11/5 and there were some discussions on 11/6 and 11/7. There were no additional comments since then. Should it continue to sit in review limbo?
>>> The standard response, and what everyone else in this position does
>>> (and there are *many*...) is to ping, asking for an update.
>> Which is why I said the approach seems fine to me (and I looked at the patch and commented on it back on 11/6). Since no one has any concrete suggestion it can be taken as LGTM. If anyone has other suggestions, they can be done after commit.
> I had concrete concerns (expressed in the code review) and suggestions
> expressed both in my review, and in person to Shuxin during the dev
> meeting. The worklist approach seems not yet correct to me. I tried to
> explain this both in the review thread and when talking in person
> during the hacking session at the dev meeting and tried to indicate
> that a follow-up email would be fine to further clarify. I was waiting
> to hear back if that had failed to make sense.
>
> All this said, the correct approach is to move this discussion to the
> code review thread until there is some consensus there.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits