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

[Hubert Tong via llvm-commits]

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/20201217/253c6df8/attachment.html>