Closed
Bug 456028
Opened 16 years ago
Closed 15 years ago
Need x86-64 Optimization for module/lcms
Categories
(Core :: Graphics: Color Management, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: m_kato, Assigned: m_kato)
References
Details
(Keywords: 64bit, fixed1.9.1)
Attachments
(1 file, 2 obsolete files)
1.90 KB,
patch
|
bholley
:
review+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
Current LCMS has 2 problems for x86-64 platform. - no SSE2 optimization. Current is for x86 only. - sizeof(DWORD) != 4. Don't use "long" as 32bit value.
Assignee | ||
Updated•16 years ago
|
Attachment #339436 -
Flags: review?(vladimir)
Attachment #339436 -
Flags: review?(vladimir)
Attachment #339436 -
Flags: review?(bholley)
Attachment #339436 -
Flags: review+
Comment on attachment 339436 [details] [diff] [review] patch v1 That looks fine, but Bobby should look over it as well.
(We should probably just use "int" in the 32-bit case as well)
Comment 3•16 years ago
|
||
Comment on attachment 339436 [details] [diff] [review] patch v1 Cheers from Buenos Aires. Patch looks good. r=me conditional on it passing the lcms test suite. I haven´t had time to put together docs and fix it up, but it´s pretty straightforward. Go to th e build directory and do ./gfxColorManagementTest check /path/to/src/gfx/thebes/test/gfxColorManagementTest.cmtest You should get a warning about 4 off-by-2 errors for one of the profiles, and everything else should pass 100%.
Attachment #339436 -
Flags: review?(bholley) → review+
Assignee | ||
Comment 4•16 years ago
|
||
I tested gfxColorManagementTest on x64 build, then, I got same result of x86.
Assignee | ||
Updated•16 years ago
|
Attachment #339436 -
Flags: superreview?(vladimir)
Attachment #339436 -
Flags: superreview?(vladimir) → superreview+
Comment on attachment 339436 [details] [diff] [review] patch v1 Looks fine to me!
Assignee | ||
Comment 6•15 years ago
|
||
I found a few problem for this patch. Build fail will occur on CentOS 5.2 for x86. I will resend new patch.
Assignee: nobody → m_kato
Status: NEW → ASSIGNED
Updated•15 years ago
|
Component: ImageLib → GFX: Color Management
Keywords: 64bit
QA Contact: imagelib → color-management
Hardware: x86 → x86_64
Comment 7•15 years ago
|
||
We ran into this over at bug 459617. Makoto - what's the status on this?
Comment 8•15 years ago
|
||
Also, given that 3.1 b3 is freezing soon, I think we should hurry this up, since I think we want it in for 3.1 (no sense in slowing down x86_64 folks unnecessarily and putting them on a less-supported code-path). Makoto, Can you either post the updated patch or say what the issues were when you read this?
Assignee | ||
Comment 9•15 years ago
|
||
Sorry. I fotgot this bug.
Attachment #339436 -
Attachment is obsolete: true
Assignee | ||
Comment 10•15 years ago
|
||
Previous patch (v2) is invalid. I will submit new patch soon.
Assignee | ||
Comment 11•15 years ago
|
||
Attachment #356308 -
Attachment is obsolete: true
Attachment #356334 -
Flags: review?(bholley)
Comment 12•15 years ago
|
||
makoto - why do we not want to have the xmm registers in the clobber list for x86?
Assignee | ||
Comment 13•15 years ago
|
||
gcc for x86 doesn't support XMM registers into destroy list. When I test on CentOS 5.2 x86 (gcc 4.1.x), original patch throws compile error. You know, x86-64 uses XMM register as floating point register. I add it to destroy list.
Updated•15 years ago
|
Attachment #356334 -
Flags: review?(bholley) → review+
Comment 14•15 years ago
|
||
Comment on attachment 356334 [details] [diff] [review] patch v2.1 Ok, r=me. I'll land this once bug 473013 is closed and the tree re-opens.
Updated•15 years ago
|
Keywords: checkin-needed
Comment 15•15 years ago
|
||
Comment on attachment 356334 [details] [diff] [review] patch v2.1 Requesting 191 approval. This bug allows people on x86_64 to get the fastpath performance optimizations I did over the summer. It doesn't add any code, just modifies the compilation logic (since x86_64 technically isn't i386, despite more or less being a superset). I think this should definitely ship with 191.
Attachment #356334 -
Flags: approval1.9.1?
Comment 16•15 years ago
|
||
pushed to mc in a1c1a8bfd19e.
Comment 17•15 years ago
|
||
Comment on attachment 356334 [details] [diff] [review] patch v2.1 a191=beltzner
Attachment #356334 -
Flags: approval1.9.1? → approval1.9.1+
Updated•15 years ago
|
Keywords: checkin-needed
Whiteboard: [needs 191 landing]
Updated•15 years ago
|
Keywords: checkin-needed
Comment 18•15 years ago
|
||
pushed to 191 branch in ab4ef0a3e868. Resolving.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs 191 landing]
Updated•15 years ago
|
Keywords: fixed1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•