[PATCH] D31123: RegisterPressure: Add operators to RegisterMaskPair

[Daniel Berlin via llvm-commits]

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/d457de78/attachment.html>