Issue 5649055: Fix SkBlitRow_opts_arm.cpp when building with NEON (Closed)
DescriptionFix SkBlitRow_opts_arm.cpp when building with NEON
Patch Set 1 #
Total comments: 1
Patch Set 2 : Updated patch #MessagesTotal messages: 11
http://codereview.appspot.com/5649055/diff/1/src/opts/SkBlitRow_opts_arm.cpp File src/opts/SkBlitRow_opts_arm.cpp (right): http://codereview.appspot.com/5649055/diff/1/src/opts/SkBlitRow_opts_arm.cpp#... src/opts/SkBlitRow_opts_arm.cpp:34: "it eq \n\t" can you provide detail on what this is for? The comment seems to just restate that it is in a #define and the instruction name. After reading the documentation, I assume that your saying the following moveq instruction is always executed in thumb mode even when the eq condition is false? Is that correct?
Sign in to reply to this message.
On 2012/02/13 14:27:14, djsollen wrote: > http://codereview.appspot.com/5649055/diff/1/src/opts/SkBlitRow_opts_arm.cpp > File src/opts/SkBlitRow_opts_arm.cpp (right): > > http://codereview.appspot.com/5649055/diff/1/src/opts/SkBlitRow_opts_arm.cpp#... > src/opts/SkBlitRow_opts_arm.cpp:34: "it eq > \n\t" > can you provide detail on what this is for? The comment seems to just restate > that it is in a #define and the instruction name. > > After reading the documentation, I assume that your saying the following moveq > instruction is always executed in thumb mode even when the eq condition is > false? Is that correct? My (very limited) understanding is that MOVEQ is both a Thumb and an ARM instruction, and can get assembled to either depending upon whether you're assembling/compiling in ARM or Thumb mode. GCC gives an error if compiling with Thumb: Error: thumb conditional instruction should be in IT block However, I'm no ARM expert... :( BTW there are also two other bug reports for this issue, if they can shed any light on the correct thing to do: http://code.google.com/p/chromium/issues/detail?id=29203 (I think I did what comment #2 suggested?) http://code.google.com/p/skia/issues/detail?id=297
Sign in to reply to this message.
I think the code that you added is correct. However, I think the comment needs to be more explicit. Can you modify the comment to something like... Thumb does not support the standard ARM conditional instructions but instead requires the 'it' instruction to signal conditional execution. Also for your reference see http://blogs.arm.com/software-enablement/293-condition-codes-3-conditional-ex... On 2012/02/13 15:06:28, gwright wrote: > On 2012/02/13 14:27:14, djsollen wrote: > > http://codereview.appspot.com/5649055/diff/1/src/opts/SkBlitRow_opts_arm.cpp > > File src/opts/SkBlitRow_opts_arm.cpp (right): > > > > > http://codereview.appspot.com/5649055/diff/1/src/opts/SkBlitRow_opts_arm.cpp#... > > src/opts/SkBlitRow_opts_arm.cpp:34: "it eq > > \n\t" > > can you provide detail on what this is for? The comment seems to just restate > > that it is in a #define and the instruction name. > > > > After reading the documentation, I assume that your saying the following moveq > > instruction is always executed in thumb mode even when the eq condition is > > false? Is that correct? > > My (very limited) understanding is that MOVEQ is both a Thumb and an ARM > instruction, and can get assembled to either depending upon whether you're > assembling/compiling in ARM or Thumb mode. GCC gives an error if compiling with > Thumb: > > Error: thumb conditional instruction should be in IT block > > However, I'm no ARM expert... :( > > BTW there are also two other bug reports for this issue, if they can shed any > light on the correct thing to do: > > http://code.google.com/p/chromium/issues/detail?id=29203 (I think I did what > comment #2 suggested?) > http://code.google.com/p/skia/issues/detail?id=297
Sign in to reply to this message.
Additionally, I wouldn't include the #define but rather state in your comment that this instruction is ignored when compiled in ARM mode. On 2012/02/13 15:24:51, djsollen wrote: > I think the code that you added is correct. However, I think the comment needs > to be more explicit. Can you modify the comment to something like... > > Thumb does not support the standard ARM conditional instructions but instead > requires the 'it' instruction to signal conditional execution. > > Also for your reference see > http://blogs.arm.com/software-enablement/293-condition-codes-3-conditional-ex... > > > On 2012/02/13 15:06:28, gwright wrote: > > On 2012/02/13 14:27:14, djsollen wrote: > > > http://codereview.appspot.com/5649055/diff/1/src/opts/SkBlitRow_opts_arm.cpp > > > File src/opts/SkBlitRow_opts_arm.cpp (right): > > > > > > > > > http://codereview.appspot.com/5649055/diff/1/src/opts/SkBlitRow_opts_arm.cpp#... > > > src/opts/SkBlitRow_opts_arm.cpp:34: "it eq > > > \n\t" > > > can you provide detail on what this is for? The comment seems to just > restate > > > that it is in a #define and the instruction name. > > > > > > After reading the documentation, I assume that your saying the following > moveq > > > instruction is always executed in thumb mode even when the eq condition is > > > false? Is that correct? > > > > My (very limited) understanding is that MOVEQ is both a Thumb and an ARM > > instruction, and can get assembled to either depending upon whether you're > > assembling/compiling in ARM or Thumb mode. GCC gives an error if compiling > with > > Thumb: > > > > Error: thumb conditional instruction should be in IT block > > > > However, I'm no ARM expert... :( > > > > BTW there are also two other bug reports for this issue, if they can shed any > > light on the correct thing to do: > > > > http://code.google.com/p/chromium/issues/detail?id=29203 (I think I did what > > comment #2 suggested?) > > http://code.google.com/p/skia/issues/detail?id=297
Sign in to reply to this message.
lgtm
Sign in to reply to this message.
On 2012/02/14 13:17:25, djsollen wrote: > lgtm Was this ever landed?
Sign in to reply to this message.
On 2012/03/28 23:00:17, gwright wrote: > On 2012/02/14 13:17:25, djsollen wrote: > > lgtm > > Was this ever landed? I don't think so, unless you landed it. If you want me to land it just let me know.
Sign in to reply to this message.
On 2012/03/29 15:26:25, djsollen wrote: > On 2012/03/28 23:00:17, gwright wrote: > > On 2012/02/14 13:17:25, djsollen wrote: > > > lgtm > > > > Was this ever landed? > > I don't think so, unless you landed it. If you want me to land it just let me > know. I can't land it - no commit access :) If you could land it for me that'd be great. Thanks!
Sign in to reply to this message.
On 2012/03/29 15:31:05, gwright wrote: > On 2012/03/29 15:26:25, djsollen wrote: > > On 2012/03/28 23:00:17, gwright wrote: > > > On 2012/02/14 13:17:25, djsollen wrote: > > > > lgtm > > > > > > Was this ever landed? > > > > I don't think so, unless you landed it. If you want me to land it just let me > > know. > > I can't land it - no commit access :) If you could land it for me that'd be > great. Thanks! The patch has been landed so feel free to close out this issue.
Sign in to reply to this message.
|
