Closed Bug 1585351 Opened 4 months ago Closed 4 months ago

Investigate Bug 1460357 and see if it can be backed out

Categories

(Firefox Build System :: General: Unsupported Platforms, task)

task
Not set

Tracking

(firefox71 fixed)

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: tjr, Assigned: tjr)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1460357 disabled AVX because gcc was generating unaligned instructions. But clang doesn't seem
to do that.

Lee, is there any chance you could point me to a test or test page that definitely uses this code so I can double confirm there's no crash?

People might still build with GCC, so are you sure you can assume we're building with clang?

(In reply to Lee Salzman [:lsalzman] from comment #3)

People might still build with GCC, so are you sure you can assume we're building with clang?

Sorry - this is specifically about gcc on Windows which we no longer support.

Well, if the MINGW32 check doesn't actually ensnare clang, there is currently no reason to remove this. It's already rolled into a much bigger patch I'm maintaining, so it would be more effort to remove than to just leave it in place.

(In reply to Lee Salzman [:lsalzman] from comment #5)

Well, if the MINGW32 check doesn't actually ensnare clang, there is currently no reason to remove this. It's already rolled into a much bigger patch I'm maintaining, so it would be more effort to remove than to just leave it in place.

Sorry - yes; the MINGW32 macro matches both mingw-gcc and mingw-clang builds so it is affecting the current mingw builds.

That said, this is - as far as I know - just a performance improvement so it would be fine to wait on this until the next time you rebase your patch and roll it up in that change.

(In reply to Tom Ritter [:tjr] from comment #6)

(In reply to Lee Salzman [:lsalzman] from comment #5)

Well, if the MINGW32 check doesn't actually ensnare clang, there is currently no reason to remove this. It's already rolled into a much bigger patch I'm maintaining, so it would be more effort to remove than to just leave it in place.

Sorry - yes; the MINGW32 macro matches both mingw-gcc and mingw-clang builds so it is affecting the current mingw builds.

That said, this is - as far as I know - just a performance improvement so it would be fine to wait on this until the next time you rebase your patch and roll it up in that change.

If that's the case just go ahead and land this and I'll roll it in.

Pushed by tritter@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b50de96607f4
Bug 1460357 disabled AVX instructions for the mingw build; this is no longer needed r=lsalzman
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
You need to log in before you can comment on or make changes to this bug.