avt77 abandoned this revision.
avt77 added a comment.
It is not a bug.
https://reviews.llvm.org/D44559
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
craig.topper added a comment.
While X86 does have multiplies that return double width results, it also has
16/32/64-bit forms of imul that only return the lower portion of the result.
Those multiplies are typically faster and have fewer uops than the double width
multiplies so we prefer to use
avt77 added a comment.
In fact I think about mul only because of X86 nature: it always put the mul
result in wider place (than its args). (Don't know about other CPUs - maybe the
same?) As result we could change the current design to reflect this feature:
8bit x 8bit -> 16bit
16bit x 16bit ->
rjmccall added a comment.
What would the design for that warning be? C promotes all arithmetic on
sub-int types to int, and if that's assigned back to a variable of the original
type, there's a conversion. But you seem to only want to warn about truncating
the result of multiplication and
avt77 added a comment.
In https://reviews.llvm.org/D44559#1049472, @rjmccall wrote:
> You are welcome to start a thread on cfe-dev to gather support for changing
> the design of -Wconversion to always warn about the potential for overflow in
> sub-int multiplication.
Maybe the stupid
rjmccall added a comment.
In https://reviews.llvm.org/D44559#1049049, @avt77 wrote:
> > If that operation overflows, so be it — we're not going to warn about the
> > potential for overflow every time the user adds two ints, and by the same
> > token, it doesn't make any sense to warn about
lebedev.ri added a comment.
In https://reviews.llvm.org/D44559#1049049, @avt77 wrote:
> ...
> But I don't mind to close the review - and the corresponding bug of course.
I disagree with closing the bug.
It's a real issue that is not detected/diagnosed by anything in clang
(diagnostic,
avt77 added a comment.
> If that operation overflows, so be it — we're not going to warn about the
> potential for overflow every time the user adds two ints, and by the same
> token, it doesn't make any sense to warn about every time the user adds two
> shorts just because the language made
rjmccall added a comment.
In https://reviews.llvm.org/D44559#1048399, @lebedev.ri wrote:
> I'm having a very hard time following this thread. In fact, i can't follow
> the logic at all.
>
> In https://reviews.llvm.org/D44559#1041007, @rjmccall wrote:
>
> > ...
> > This patch, however, is
lebedev.ri added a comment.
I'm having a very hard time following this thread. In fact, i can't follow the
logic at all.
In https://reviews.llvm.org/D44559#1041007, @rjmccall wrote:
> ...
> This patch, however, is contrary to the design of -Wconversion, which does
> attempt to avoid warning
rjmccall added a comment.
In https://reviews.llvm.org/D44559#1048085, @avt77 wrote:
> In https://reviews.llvm.org/D44559#1045758, @rjmccall wrote:
>
> > No, I still oppose this patch.
>
>
> OK, we have 2 possibilities here (fmpov):
>
> 1. Forget about the issue and don't do anything now - it is
avt77 added a comment.
In https://reviews.llvm.org/D44559#1045758, @rjmccall wrote:
> No, I still oppose this patch.
OK, we have 2 possibilities here (fmpov):
1. Forget about the issue and don't do anything now - it is not a bug
2. Return the width based on real analyze of mul args:
-
rjmccall requested changes to this revision.
rjmccall added a comment.
This revision now requires changes to proceed.
No, I still oppose this patch.
https://reviews.llvm.org/D44559
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
avt77 added a comment.
> That's an interesting question. In general, these warnings do try to ignore
> the effects of implicit promotion. We would not want -Wsign-conversion to
> fire on `unsigned short x = an_unsigned_short + 1;` (or `- 1`, for that
> matter), even though formally this
rjmccall added a comment.
In https://reviews.llvm.org/D44559#1044653, @aaron.ballman wrote:
> In https://reviews.llvm.org/D44559#1044639, @rjmccall wrote:
>
> > In https://reviews.llvm.org/D44559#1044186, @avt77 wrote:
> >
> > > >> In https://reviews.llvm.org/D44559#1040799, @rjmccall wrote:
> >
aaron.ballman added a comment.
In https://reviews.llvm.org/D44559#1044639, @rjmccall wrote:
> In https://reviews.llvm.org/D44559#1044186, @avt77 wrote:
>
> > >> In https://reviews.llvm.org/D44559#1040799, @rjmccall wrote:
> > >>
> > >>> I think we're correct not to warn here and that GCC/ICC
rjmccall added a comment.
In https://reviews.llvm.org/D44559#1044186, @avt77 wrote:
> >> In https://reviews.llvm.org/D44559#1040799, @rjmccall wrote:
> >>
> >>> I think we're correct not to warn here and that GCC/ICC are being noisy.
> >>> The existence of a temporary promotion to a wider
avt77 updated this revision to Diff 139271.
avt77 added a comment.
I removed the dependence on arch and updated the tests.
https://reviews.llvm.org/D44559
Files:
lib/Sema/SemaChecking.cpp
test/Sema/conversion.c
test/SemaCXX/conversion.cpp
Index: test/SemaCXX/conversion.cpp
avt77 added a comment.
>> In https://reviews.llvm.org/D44559#1040799, @rjmccall wrote:
>>
>>> I think we're correct not to warn here and that GCC/ICC are being noisy.
>>> The existence of a temporary promotion to a wider type doesn't justify
>>> warning on arithmetic between two operands that
rjmccall added a comment.
In https://reviews.llvm.org/D44559#1041002, @lebedev.ri wrote:
> In https://reviews.llvm.org/D44559#1041001, @rjmccall wrote:
>
> > In https://reviews.llvm.org/D44559#1040928, @lebedev.ri wrote:
> >
> > > In https://reviews.llvm.org/D44559#1040799, @rjmccall wrote:
> >
lebedev.ri added a comment.
In https://reviews.llvm.org/D44559#1041001, @rjmccall wrote:
> In https://reviews.llvm.org/D44559#1040928, @lebedev.ri wrote:
>
> > In https://reviews.llvm.org/D44559#1040799, @rjmccall wrote:
> >
> > > I think we're correct not to warn here and that GCC/ICC are being
rjmccall added a comment.
In https://reviews.llvm.org/D44559#1040928, @lebedev.ri wrote:
> In https://reviews.llvm.org/D44559#1040799, @rjmccall wrote:
>
> > I think we're correct not to warn here and that GCC/ICC are being noisy.
> > The existence of a temporary promotion to a wider type
lebedev.ri added a comment.
In https://reviews.llvm.org/D44559#1040799, @rjmccall wrote:
> I think we're correct not to warn here and that GCC/ICC are being noisy. The
> existence of a temporary promotion to a wider type doesn't justify warning on
> arithmetic between two operands that are
rjmccall added a comment.
I think we're correct not to warn here and that GCC/ICC are being noisy. The
existence of a temporary promotion to a wider type doesn't justify warning on
arithmetic between two operands that are the same size as the ultimate result.
It is totally fair for users to
craig.topper added a comment.
icc seems to match gcc for those last 4 cases i sent. And MSVC is throwing an
odd signed/unsigned mismatch
https://godbolt.org/g/s5FUTv
Repository:
rC Clang
https://reviews.llvm.org/D44559
___
cfe-commits mailing
rjmccall added a comment.
The purpose of this analysis is not to compute the theoretical information
content of the computation result. We are conservative about bounds precisely
so that we do not warn in situations like these.
Repository:
rC Clang
https://reviews.llvm.org/D44559
aaron.ballman added a comment.
In https://reviews.llvm.org/D44559#1040435, @craig.topper wrote:
> Fair point, what is the default signedness of char?
It's decided by target architecture. ARM and PPC often use unsigned, x86 often
uses signed.
> FWIW, all these warn in gcc. So they seem to be
craig.topper added a comment.
Fair point, what is the default signedness of char?
FWIW, all these warn in gcc. So they seem to be just checking purely based on
the int promotion without any concern for the original size?
unsigned short foo(unsigned char a) {
return a * a;
}
aaron.ballman added a comment.
In https://reviews.llvm.org/D44559#1040398, @craig.topper wrote:
> gcc also warns for this
>
> short foo(char a) {
>
> return a * a;
>
> }
>
> Despite the fact that the char would be promoted to int, the upper bits are
> just sign bits and the multiply result
craig.topper added a comment.
gcc also warns for this
short foo(char a) {
return a * a;
}
Despite the fact that the char would be promoted to int, the upper bits are
just sign bits and the multiply result still fits in a short.
Repository:
rC Clang
https://reviews.llvm.org/D44559
lebedev.ri added inline comments.
Comment at: lib/Sema/SemaChecking.cpp:8541
+ switch (C.getTargetInfo().getTriple().getArch()) {
+ case llvm::Triple::x86:
+ case llvm::Triple::x86_64: {
I agree with @RKSimon, i don't see why this would be
31 matches
Mail list logo
