[PATCH] D31123: RegisterPressure: Add operators to RegisterMaskPair

[Daniel Berlin via llvm-commits]

You can edit the revision summary still, and it should still do it :)


On Sun, Mar 19, 2017 at 3:28 PM, Axel Davy <axel.davy at ens.fr> wrote:

> So that's how to do it :-)
>
> I guess it's too late to add it, but I'll think of it next time, thanks !
>
>
> On 19/03/2017 23:26, Daniel Berlin wrote:
>
> Also, if you add the text "depends on <reviewnumber>" to the summary of
> the patch, and phab will link them.
>
>
>
> On Sun, Mar 19, 2017 at 3:22 PM, Daniel Berlin <dberlin at dberlin.org>
> wrote:
>
>>
>>
>> On Sun, Mar 19, 2017 at 3:13 PM, Axel Davy <axel.davy at ens.fr> wrote:
>>
>>> The patch is on top of other patches not yet merged.
>>>
>>> You can find a branch with all the patches here:
>>> https://github.com/axeldavy/llvm/commits/llvm_upstream
>>>
>>> getPairsForRegs is not equivalent to std::copy.
>>>
>>
>> Okay, then it's definitely equivalent to std::transform  ;)
>> (though note: It looks nearly trivial to define a subclass of the
>> register mask pair that makes *all* those calls std::copy)
>>
>>
>>> The name is probably poorly choosen,
>>> I'm open to suggestions.
>>>
>>> Yours,
>>>
>>> Axel
>>>
>>>
>>> On 19/03/2017 23:10, Daniel Berlin wrote:
>>>
>>> This code has a number of issues (using copying all over the place) that
>>> is likely causing your issue.
>>> For example:
>>> // Idem for a list of Reg/Mask
>>>   SmallVector<RegisterMaskPair, 8> getPairsForRegs(
>>>     const SmallVector<RegisterMaskPair, 8> Regs);
>>>
>>>
>>> You almost certainly meant this to be a const reference to Regs.
>>> etc
>>>
>>> Actually, staring at/ this harder, that function is just equivalent to
>>> std::copy, no?
>>>
>>> I tried to patch it into a client, but it does not patch cleanly.
>>> If you rebase it, i'll take a closer look.
>>>
>>>
>>> On Sun, Mar 19, 2017 at 2:22 PM, Axel Davy <axel.davy at ens.fr> wrote:
>>>
>>>> Here is the code that makes use of this:
>>>>
>>>> https://reviews.llvm.org/D31124
>>>>
>>>> Axel
>>>>
>>>>
>>>> On 19/03/2017 22:16, Daniel Berlin wrote:
>>>>
>>>> Can you give the actual code you are trying to use?
>>>> It should work fine in, say, densemap, if you define the densemapinfo
>>>> for it.
>>>>
>>>> Past that, without seeing exactly how you are trying to use it, it is
>>>> hard to say what the problem is.
>>>>
>>>> I would venture a guess if you are doing it in a way that it is trying
>>>> to default construct these, you are probably not doing it right :)
>>>>
>>>>
>>>> On Sun, Mar 19, 2017 at 12:44 PM, Axel Davy via llvm-commits <
>>>> llvm-commits at lists.llvm.org> wrote:
>>>>
>>>>> This is similar to just allocating the structure.
>>>>>
>>>>> Somehow to use maps, it complained without this constructor.
>>>>> I don't think however any possible initialization value would make
>>>>> sense.
>>>>>
>>>>> Perhaps UINT_MAX for Reg and 0 for LaneMask ? To clearly indicate
>>>>> there is error
>>>>> if you see these in logs ?
>>>>>
>>>>> Axel
>>>>>
>>>>>
>>>>> On 19/03/2017 20:23, Stanislav Mekhanoshin via Phabricator wrote:
>>>>>
>>>>>> rampitec added inline comments.
>>>>>>
>>>>>>
>>>>>> ================
>>>>>> Comment at: include/llvm/CodeGen/RegisterPressure.h:45
>>>>>> +
>>>>>> +  RegisterMaskPair() {}
>>>>>> +
>>>>>> ----------------
>>>>>> That is undesirable to have a constructor which does not initialize
>>>>>> fields.
>>>>>>
>>>>>>
>>>>>> Repository:
>>>>>>    rL LLVM
>>>>>>
>>>>>> https://reviews.llvm.org/D31123
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at lists.llvm.org
>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170319/ecfe9887/attachment-0001.html>