[llvm] 15f2d4f - [AIX] Fixed "comparison of unsigned expression >= 0 is always true" gcc warnings.

[David Blaikie via llvm-commits]

On Fri, Dec 18, 2020 at 10:06 AM digger lin <digger.llvm at gmail.com> wrote:

> added  a new NFC patch
> https://reviews.llvm.org/rGd551e40f1cff7a63218f34112bd0dddaa2b12dbb to
> address the comment.
>

Thanks!

Though looking at it, the original code seems to have been buggy, rather
than just redundant, right? (the condition would never have fired) - the
code under the "if" condition hasn't been tested? (since if it had, it
would've shown the code to be unreachable) - Could you/someone add the
missing test coverage here?


>
> On Thu, Dec 17, 2020 at 9:17 PM Hubert Tong <
> hubert.reinterpretcast at gmail.com> wrote:
>
>> On Wed, Dec 16, 2020 at 8:47 PM David Blaikie <dblaikie at gmail.com> wrote:
>>
>>>
>>>
>>> On Tue, Dec 15, 2020 at 9:24 AM Hubert Tong <
>>> hubert.reinterpretcast at gmail.com> wrote:
>>>
>>>> On Mon, Dec 14, 2020 at 8:36 PM David Blaikie via llvm-commits <
>>>> llvm-commits at lists.llvm.org> wrote:
>>>>
>>>>> Any idea if this is the most suitable fix? Is "Bits" representing
>>>>> negative values? If not, perhaps it'd be more appropriate to remove the >=
>>>>> 0 comparison, rather than to change the type?
>>>>>
>>>> For context, this aspect of the implementation will likely be replaced
>>>> as further functionality (to implement the encoding form when vector
>>>> parameters are present) is implemented. In terms of NFC evolution of this
>>>> code, the reparsing loop is not ideal. If the code was not being changed
>>>> anyway, it would make sense to store the "next position" alongside the
>>>> encoded bitstring.
>>>>
>>>> As for the signedness of the type: I believe changing the type is
>>>> appropriate in this case even if the comparison is modified to avoid the
>>>> subtraction.
>>>>
>>>
>>> Appropriate in that you think it's a more suitable type for the value
>>> being stored in it, even if it weren't for the warning?
>>>
>> There's been quite the thread that has not gotten consensus one way or
>> the other about preferring `int` (see related:
>> https://reviews.llvm.org/D63049). The variable here (aside from
>> representing something that is never negative) has no particular reason to
>> be `unsigned`. So, using `int` to avoid the wraparound at `0` has its
>> advantages. Perhaps I would give the rationale of using `unsigned` more
>> weight if this was part of a function interface.
>>
>>
>>>
>>>
>>>> I do agree that changing the comparison should also be done; however,
>>>> the change to the type was the easiest change to eyeball in the context of
>>>> responding to the breakage.
>>>>
>>>
>>> Would be good to clean that up, then - could someone make that change (&
>>> mention the commit hash here)? (otherwise it'll be hard to track down the
>>> longer the code evolves - without the warning to point to it)
>>>
>> @digger lin <digger.llvm at gmail.com>, can you post this change or (if the
>> code is ready) the change that removes the current implementation (in order
>> to add functionality)?
>>
>>
>>>
>>>
>>>>
>>>>
>>>>>
>>>>> On Mon, Dec 14, 2020 at 8:09 AM via llvm-commits <
>>>>> llvm-commits at lists.llvm.org> wrote:
>>>>>
>>>>>>
>>>>>> Author: diggerlin
>>>>>> Date: 2020-12-14T11:08:40-05:00
>>>>>> New Revision: 15f2d4f198380762e9fcf6b456d405078b87ae7a
>>>>>>
>>>>>> URL:
>>>>>> https://github.com/llvm/llvm-project/commit/15f2d4f198380762e9fcf6b456d405078b87ae7a
>>>>>> DIFF:
>>>>>> https://github.com/llvm/llvm-project/commit/15f2d4f198380762e9fcf6b456d405078b87ae7a.diff
>>>>>>
>>>>>> LOG: [AIX] Fixed "comparison of unsigned expression >= 0 is always
>>>>>> true" gcc warnings.
>>>>>>
>>>>>> Summary:
>>>>>>
>>>>>> fixed a  Fixed "comparison of unsigned expression >= 0 is always
>>>>>> true" gcc warnings.
>>>>>> http://lab.llvm.org:8011/#/builders/5/builds/2407/steps/2/logs/stdio
>>>>>>
>>>>>> the error caused by patch https://reviews.llvm.org/D92398
>>>>>>
>>>>>> Added:
>>>>>>
>>>>>>
>>>>>> Modified:
>>>>>>     llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
>>>>>>
>>>>>> Removed:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> ################################################################################
>>>>>> diff  --git a/llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
>>>>>> b/llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
>>>>>> index 02a425044c75..d364eb9d3996 100644
>>>>>> --- a/llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
>>>>>> +++ b/llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp
>>>>>> @@ -67,7 +67,7 @@ bool PPCFunctionInfo::isLiveInZExt(Register VReg)
>>>>>> const {
>>>>>>
>>>>>>  void PPCFunctionInfo::appendParameterType(ParamType Type) {
>>>>>>    uint32_t CopyParamType = ParameterType;
>>>>>> -  unsigned Bits = 0;
>>>>>> +  int Bits = 0;
>>>>>>
>>>>>>    // If it is fixed type, we only need to increase the
>>>>>> FixedParamNum, for
>>>>>>    // the bit encode of fixed type is bit of zero, we do not need to
>>>>>> change the
>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> llvm-commits mailing list
>>>>>> llvm-commits at lists.llvm.org
>>>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>>>>
>>>>> _______________________________________________
>>>>> llvm-commits mailing list
>>>>> llvm-commits at lists.llvm.org
>>>>> https://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/20201218/3383f7df/attachment.html>