Closed Bug 456028 Opened 16 years ago Closed 15 years ago

Need x86-64 Optimization for module/lcms

Categories

(Core :: Graphics: Color Management, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: m_kato, Assigned: m_kato)

References

Details

(Keywords: 64bit, fixed1.9.1)

Attachments

(1 file, 2 obsolete files)

Attached patch patch v1 (obsolete) — 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.
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 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+
I tested gfxColorManagementTest on x64 build, then, I got same result of x86.
Attachment #339436 - Flags: superreview?(vladimir)
Attachment #339436 - Flags: superreview?(vladimir) → superreview+
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
Component: ImageLib → GFX: Color Management
Keywords: 64bit
QA Contact: imagelib → color-management
Hardware: x86 → x86_64
Blocks: 459617
We ran into this over at bug 459617.

Makoto - what's the status on this?
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?
Attached patch patch v2 (obsolete) — Splinter Review
Sorry.  I fotgot this bug.
Attachment #339436 - Attachment is obsolete: true
Previous patch (v2) is invalid.  I will submit new patch soon.
Attached patch patch v2.1Splinter Review
Attachment #356308 - Attachment is obsolete: true
Attachment #356334 - Flags: review?(bholley)
makoto - why do we not want to have the xmm registers in the clobber list for x86?
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.
Attachment #356334 - Flags: review?(bholley) → review+
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.
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?
pushed to mc in a1c1a8bfd19e.
Comment on attachment 356334 [details] [diff] [review]
patch v2.1

a191=beltzner
Attachment #356334 - Flags: approval1.9.1? → approval1.9.1+
Keywords: checkin-needed
Whiteboard: [needs 191 landing]
Keywords: checkin-needed
pushed to 191 branch in ab4ef0a3e868. Resolving.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs 191 landing]
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.