4 ms·

The key here is 'unnecessarily'. If it it works just as well on an AMD CPU it should be enabled on an AMD CPU regardless of who submitted it.

jcelerier 7y ago
but it is to AMD engineers's job or at least people willing to run benchmarks on AMD cpus, to ensure that "it works just as well on an AMD CPU", not to Intel.
- arghwhat 7y ago
  No. With an open source project like this, every contributor has a responsibility towards all users of that code. That means that if Intel makes a change, they are also responsible for AMD support. Putting a generic optimization behind a vendor detection is not acceptable in the community. However, as others' have mentioned, this previously might not have benefited AMD, despite technically being supported, so it was likely meant well at the time of implementation. Intel is usually quite good players when it comes to open source contributions.
  - rat9988 7y ago
    They didn't harm in any way the AMD users, just didn't invest in their well being. You can't expect intel to benchmark if a feature would be better or not on amd, that's not its role. It submitted a patch that was good for its users. Up to amd to benchmark it for their users. It would still be a lot less expensive to benchmark it and enable it for amd, than develop it from scratch as intel.
    - gtirloni 7y ago
      That's true. Whoever accepted the patches should have thought of that. But that happens, right? I don't think this is newsworthy at all. Bad implementations happen, people find better ways, they refactor and improve things. This holy war some people want to have between AMD and Intel is quite boring.
    - admax88q 7y ago
      > You can't expect intel to benchmark if a feature would be better or not on amd, that's not its role. Why not? If I was maintainer I wouldn't accept the patch, I'd ask the authors to test on AMD as well. Intel is well funded and glibc is not their project. If they want glibc to include optimizations for their platform they can do the minimal level of effort to see if it can be enabled on AMD as well.
      - cptskippy 7y ago
        Historically Intel has been found to create suboptimal code paths in its own compiler, in addition to not enabling features supported on competitor's platform, in its own compiler. What happens if the implementation is slower but still works on AMD? Is Intel responsible for also performance testing and determining if/when to disable an implementation on a given chip? You're putting a lot of burden on Intel to do extensive testing and also not protecting them from criticism if a change is suboptimal for a competitor. I think it's fine to submit a patch that's known to be good for a subset of CPUs and perhaps it should be tagged for another maintainer (e.g. AMD) to review and contribute to as well.
        admax88q 7y ago
        If the original patch contained a benchmark showing that it was slower on the current AMD processors then this would be a reasonable argument. It did not, because no such testing was done. I think there's a pretty low baseline of effort they can put in. I also think that if they enabled new code paths for newer instructions that turned out to be slower on AMD, very few people would claim this was Intel being evil. Most would blame AMD for selling a defective product.
        cptskippy 7y ago
        I would argue it's absurd ask for Intel to maintain test platforms for a competitor. At best you could ask them to flag platform specific code in such a way that others, who are better equipped, can test against other platforms.
      - 7y ago
    - arghwhat 7y ago
      > You can't expect intel to benchmark if a feature would be better or not on amd, that's not its role. It is, in facts, its role as a major contributor. And they do, in fact, benchmark and fix AMD as part of its open source efforts, as they should. Case in point: https://github.com/OpenVisualCloud/SVT-VP9/pull/48 https://github.com/OpenVisualCloud/SVT-VP9/pull/48 (Intel developer contributing AVX2 improvements, specifically mentioned as AMD Epyc improvements). They can cater only to their own devices when the code only applies to those devices (e.g. the i915 graphics driver). In generic code paths, they must cater to all users. Adding optimization using generic features that have generic flags, but hiding them behind vendor detection, is borderline malicious. The developers here are well aware that there is a feature flag they should check instead.
  - jcelerier 7y ago
    > With an open source project like this, every contributor has a responsibility towards all users of that code. What ? no. Do IBM engineers have responsibility towards Qualcomm engineers when they commit IBM patches under IBM-named flags to the linux kernel ? What happens if there's a new CPU company in two years that would also happen to work fine with these flags ?
    - arghwhat 7y ago
      > Do IBM engineers have responsibility towards Qualcomm engineers Yes, in every way. However, with Qualcomm not having any PowerPC architectures, there isn't much harm that could be done. And for reference, Intel also tests AMD as they should, and even submits performance improvements for AMD as they should. But this is quite bad: Instead of: `if (supports(generic_feature)) { do_with_generic_feature(); } else { slow_approach(); }`, they did `if (intel_haswell) { do_with_generic_feature(); } else { slow_approach(); }`. The only time where that is acceptable from as big a contributor as Intel, is if they tested and concluded that other CPUs were actually slower using this feature. > What happens if there's a new CPU company in two years that would also happen to work fine with these flags ? That is exactly why the feature flags exist in this generic code! You can query what instruction sets are available on the CPU. Vendor-specific code should only be added to deal with product-specific defect.
  - codewiz 7y ago
    The glibc core maintainers have such responsibility, yes, but it's too much burden for occasional contributors focusing on a single issue or a particular architecture. Setting the bar too high would make glibc lose valuable patches from both Intel and AMD. A reasonable compromise is requiring architecture-specific contributions to at least do no harm to other vendors, and to not increase maintenance costs for core developers by duplicating code. In this light, AMD engineers wanting to enable the haswell optimizations for their processors would be asked to share the existing code rather than copy-paste it. Intel engineers would participate in the public review to ensure AMD patches don't cause regressions for Haswell. If they have contributed testcases, they will demand that AMD patches pass them on all supported architectures before being merged. This is pretty standard in all open source projects with multiple stakeholders.
    - arghwhat 7y ago
      > occasional contributors Intel has made 171 contributions in form of commits to glibc as of master today. I doubt they can be considered an "occasional contributor". And even then, small contributions only get to bypass the responsibility if we're dealing with small bugfixes.
- rocqua 7y ago
  If intel is intentionally checking based on vendor rather than feature, that is scummy. Glibc should not accept patches that do this check. Instead requiring such checks to be based on feature detection.
- lidHanteyk 7y ago
  Yes, and that's what is happening with this glibc improvement. glibc is part of the C runtime; it is part of what makes C run on a given CPU. What are you complaining about, exactly? The system is working as it ought to work.

jcelerier 7y ago

but it is to AMD engineers's job or at least people willing to run benchmarks on AMD cpus, to ensure that "it works just as well on an AMD CPU", not to Intel.

arghwhat 7y ago
No. With an open source project like this, every contributor has a responsibility towards all users of that code. That means that if Intel makes a change, they are also responsible for AMD support. Putting a generic optimization behind a vendor detection is not acceptable in the community. However, as others' have mentioned, this previously might not have benefited AMD, despite technically being supported, so it was likely meant well at the time of implementation. Intel is usually quite good players when it comes to open source contributions.
- rat9988 7y ago
  They didn't harm in any way the AMD users, just didn't invest in their well being. You can't expect intel to benchmark if a feature would be better or not on amd, that's not its role. It submitted a patch that was good for its users. Up to amd to benchmark it for their users. It would still be a lot less expensive to benchmark it and enable it for amd, than develop it from scratch as intel.
  - gtirloni 7y ago
    That's true. Whoever accepted the patches should have thought of that. But that happens, right? I don't think this is newsworthy at all. Bad implementations happen, people find better ways, they refactor and improve things. This holy war some people want to have between AMD and Intel is quite boring.
  - admax88q 7y ago
    > You can't expect intel to benchmark if a feature would be better or not on amd, that's not its role. Why not? If I was maintainer I wouldn't accept the patch, I'd ask the authors to test on AMD as well. Intel is well funded and glibc is not their project. If they want glibc to include optimizations for their platform they can do the minimal level of effort to see if it can be enabled on AMD as well.
    - cptskippy 7y ago
      Historically Intel has been found to create suboptimal code paths in its own compiler, in addition to not enabling features supported on competitor's platform, in its own compiler. What happens if the implementation is slower but still works on AMD? Is Intel responsible for also performance testing and determining if/when to disable an implementation on a given chip? You're putting a lot of burden on Intel to do extensive testing and also not protecting them from criticism if a change is suboptimal for a competitor. I think it's fine to submit a patch that's known to be good for a subset of CPUs and perhaps it should be tagged for another maintainer (e.g. AMD) to review and contribute to as well.
      - admax88q 7y ago
        If the original patch contained a benchmark showing that it was slower on the current AMD processors then this would be a reasonable argument. It did not, because no such testing was done. I think there's a pretty low baseline of effort they can put in. I also think that if they enabled new code paths for newer instructions that turned out to be slower on AMD, very few people would claim this was Intel being evil. Most would blame AMD for selling a defective product.
        cptskippy 7y ago
        I would argue it's absurd ask for Intel to maintain test platforms for a competitor. At best you could ask them to flag platform specific code in such a way that others, who are better equipped, can test against other platforms.
    - 7y ago
  - arghwhat 7y ago
    > You can't expect intel to benchmark if a feature would be better or not on amd, that's not its role. It is, in facts, its role as a major contributor. And they do, in fact, benchmark and fix AMD as part of its open source efforts, as they should. Case in point: https://github.com/OpenVisualCloud/SVT-VP9/pull/48 https://github.com/OpenVisualCloud/SVT-VP9/pull/48 (Intel developer contributing AVX2 improvements, specifically mentioned as AMD Epyc improvements). They can cater only to their own devices when the code only applies to those devices (e.g. the i915 graphics driver). In generic code paths, they must cater to all users. Adding optimization using generic features that have generic flags, but hiding them behind vendor detection, is borderline malicious. The developers here are well aware that there is a feature flag they should check instead.
- jcelerier 7y ago
  > With an open source project like this, every contributor has a responsibility towards all users of that code. What ? no. Do IBM engineers have responsibility towards Qualcomm engineers when they commit IBM patches under IBM-named flags to the linux kernel ? What happens if there's a new CPU company in two years that would also happen to work fine with these flags ?
  - arghwhat 7y ago
    > Do IBM engineers have responsibility towards Qualcomm engineers Yes, in every way. However, with Qualcomm not having any PowerPC architectures, there isn't much harm that could be done. And for reference, Intel also tests AMD as they should, and even submits performance improvements for AMD as they should. But this is quite bad: Instead of: `if (supports(generic_feature)) { do_with_generic_feature(); } else { slow_approach(); }`, they did `if (intel_haswell) { do_with_generic_feature(); } else { slow_approach(); }`. The only time where that is acceptable from as big a contributor as Intel, is if they tested and concluded that other CPUs were actually slower using this feature. > What happens if there's a new CPU company in two years that would also happen to work fine with these flags ? That is exactly why the feature flags exist in this generic code! You can query what instruction sets are available on the CPU. Vendor-specific code should only be added to deal with product-specific defect.
- codewiz 7y ago
  The glibc core maintainers have such responsibility, yes, but it's too much burden for occasional contributors focusing on a single issue or a particular architecture. Setting the bar too high would make glibc lose valuable patches from both Intel and AMD. A reasonable compromise is requiring architecture-specific contributions to at least do no harm to other vendors, and to not increase maintenance costs for core developers by duplicating code. In this light, AMD engineers wanting to enable the haswell optimizations for their processors would be asked to share the existing code rather than copy-paste it. Intel engineers would participate in the public review to ensure AMD patches don't cause regressions for Haswell. If they have contributed testcases, they will demand that AMD patches pass them on all supported architectures before being merged. This is pretty standard in all open source projects with multiple stakeholders.
  - arghwhat 7y ago
    > occasional contributors Intel has made 171 contributions in form of commits to glibc as of master today. I doubt they can be considered an "occasional contributor". And even then, small contributions only get to bypass the responsibility if we're dealing with small bugfixes.
rocqua 7y ago
If intel is intentionally checking based on vendor rather than feature, that is scummy. Glibc should not accept patches that do this check. Instead requiring such checks to be based on feature detection.
lidHanteyk 7y ago
Yes, and that's what is happening with this glibc improvement. glibc is part of the C runtime; it is part of what makes C run on a given CPU. What are you complaining about, exactly? The system is working as it ought to work.