Bug 152955

Created attachment 268639 [details] the patch

Attachment 268639 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/testb3.cpp:9014: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9032: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9051: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9070: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] Total errors found: 4 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

This is a 2x speed-up on AsmBench/FloatMM. It's neutral elsewhere.

Comment on attachment 268639 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=268639&action=review I have a few questions, maybe I'm misunderstanding how some things. > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:809 > + case Add: Wouldn't this produce zero? > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:810 > + case Sub: is this signed or unsigned subtraction? If it's signed, why is this? If it's unsigned, wouldn't this just be zero? > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:813 > + case BitAnd: > + case BitOr: > + case BitXor: Wouldn't these produce zero?

(In reply to comment #4) > Comment on attachment 268639 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=268639&action=review > > I have a few questions, maybe I'm misunderstanding how some things. > > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:809 > > + case Add: > > Wouldn't this produce zero? > > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:810 > > + case Sub: > > is this signed or unsigned subtraction? If it's signed, why is this? > If it's unsigned, wouldn't this just be zero? Never mind about signedness, my question is just if this would always be zero? > > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:813 > > + case BitAnd: > > + case BitOr: > > + case BitXor: > > Wouldn't these produce zero?

(In reply to comment #4) > Comment on attachment 268639 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=268639&action=review > > I have a few questions, maybe I'm misunderstanding how some things. > > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:809 > > + case Add: > > Wouldn't this produce zero? The expression in question is: static_cast<int32_t>(x + const) Where "static_cast<int32>()" is Trunc and "x + const" is Add. In this case, if const1 does not have any of its low 32 bits set, we are guaranteed that the expression is equivalent to: static_cast<int32_t>(x) That's guaranteed because if const has zero in all of its low 32 bits, then "x + const" does not change anything about the low 32 bits of x. That also means that this does not produce zero. It does not produce zero because it simply produces static_cast<int32>(x). > > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:810 > > + case Sub: > > is this signed or unsigned subtraction? If it's signed, why is this? > If it's unsigned, wouldn't this just be zero? There is no difference between signed subtraction and unsigned subtraction. > > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:813 > > + case BitAnd: > > + case BitOr: > > + case BitXor: > > Wouldn't these produce zero? No. If you do: static_cast<int32_t>(x | 0xabcdef1200000000) then it's equivalent to: static_cast<int32_t>(x) Because the "|" operation could only have affected the top bits. That's why there is a check that the constant has zero in all of its low bits. If it has zero in all of its low bits, then we can short-circuit the Trunc around the "|". The same is true for "^" and "&".

(In reply to comment #5) > (In reply to comment #4) > > Comment on attachment 268639 [details] > > the patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=268639&action=review > > > > I have a few questions, maybe I'm misunderstanding how some things. > > > > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:809 > > > + case Add: > > > > Wouldn't this produce zero? > > > > > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:810 > > > + case Sub: > > > > is this signed or unsigned subtraction? If it's signed, why is this? > > If it's unsigned, wouldn't this just be zero? > Never mind about signedness, my question is just if this would > always be zero? Answered above.

Comment on attachment 268639 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=268639&action=review >>>>> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:809 >>>>> + case Add: >>>> >>>> Wouldn't this produce zero? >>> >>> Never mind about signedness, my question is just if this would >>> always be zero? >> >> The expression in question is: >> >> static_cast<int32_t>(x + const) >> >> Where "static_cast<int32>()" is Trunc and "x + const" is Add. >> >> In this case, if const1 does not have any of its low 32 bits set, we are guaranteed that the expression is equivalent to: >> >> static_cast<int32_t>(x) >> >> That's guaranteed because if const has zero in all of its low 32 bits, then "x + const" does not change anything about the low 32 bits of x. >> >> That also means that this does not produce zero. It does not produce zero because it simply produces static_cast<int32>(x). > > Answered above. I see. I had a total brain fart when I was reasoning about this. Thanks for the clarification. It makes sense. >>>> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:813 >>>> + case BitXor: >>> >>> Wouldn't these produce zero? >> >> > > No. If you do: > > static_cast<int32_t>(x | 0xabcdef1200000000) > > then it's equivalent to: > > static_cast<int32_t>(x) > > Because the "|" operation could only have affected the top bits. That's why there is a check that the constant has zero in all of its low bits. If it has zero in all of its low bits, then we can short-circuit the Trunc around the "|". > > The same is true for "^" and "&". I agree with BitOr and BitXor. For BitAnd, this seems wrong. If we're ANDing something with something else with all lower 32-bits are zero, we should transform this into zero.

(In reply to comment #8) > Comment on attachment 268639 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=268639&action=review > > >>>>> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:809 > >>>>> + case Add: > >>>> > >>>> Wouldn't this produce zero? > >>> > >>> Never mind about signedness, my question is just if this would > >>> always be zero? > >> > >> The expression in question is: > >> > >> static_cast<int32_t>(x + const) > >> > >> Where "static_cast<int32>()" is Trunc and "x + const" is Add. > >> > >> In this case, if const1 does not have any of its low 32 bits set, we are guaranteed that the expression is equivalent to: > >> > >> static_cast<int32_t>(x) > >> > >> That's guaranteed because if const has zero in all of its low 32 bits, then "x + const" does not change anything about the low 32 bits of x. > >> > >> That also means that this does not produce zero. It does not produce zero because it simply produces static_cast<int32>(x). > > > > Answered above. > > I see. I had a total brain fart when I was reasoning about this. Thanks for > the clarification. It makes sense. > > >>>> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:813 > >>>> + case BitXor: > >>> > >>> Wouldn't these produce zero? > >> > >> > > > > No. If you do: > > > > static_cast<int32_t>(x | 0xabcdef1200000000) > > > > then it's equivalent to: > > > > static_cast<int32_t>(x) > > > > Because the "|" operation could only have affected the top bits. That's why there is a check that the constant has zero in all of its low bits. If it has zero in all of its low bits, then we can short-circuit the Trunc around the "|". > > > > The same is true for "^" and "&". > > I agree with BitOr and BitXor. > For BitAnd, this seems wrong. If we're ANDing something with something else > with all lower 32-bits are zero, we should transform this into zero. Good point. I'll remove the BitAnd from the switch statement.

Created attachment 268667 [details] the patch

Attachment 268667 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/b3/testb3.cpp:9015: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9033: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9052: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] ERROR: Source/JavaScriptCore/b3/testb3.cpp:9071: Consider using CHECK_EQ instead of CHECK(a == b) [readability/check] [2] Total errors found: 4 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 268667 [details] the patch r=me

Landed in http://trac.webkit.org/changeset/194836