[PATCH] D69189: [Builtins] Fix bug where powerpc builtins specializations didn't remove generic implementations.

Tue Oct 29 17:14:09 PDT 2019

delcypher added a comment.

In D69189#1724545 <https://reviews.llvm.org/D69189#1724545>, @nemanjai wrote:

> One of us here at IBM would gladly try this out on ppc64/ppc64le if that is desired. We are certainly interested in helping to ensure our target does not put undue burden on the community, but I don't think anyone here has much CMake expertise.
>  So if you let us know if you'd like us to test something out for PPC, we'll be happy to do it.

If you could test it that would be great. My current understanding of the bug I found is that on the `master` branch when the powerpc builtin library is built it will accidently contain two implementations of the `divtc3` builtin. This just so happens to workout because we're producing a static archive and the `ar` tool doesn't care that there are duplicate symbols. When the linker uses the archive it'll just pick the first symbol it finds. I think the first symbol will be the powerpc version but I'm not sure because I don't have any powerpc hardware to test this.

If you apply this patch you should hopefully find that the duplicate `divtc3` builtin symbol goes away and the only one left is the `ppc/divtc3.c` implementation.

> Just as an example, with my very limited knowledge of CMake, I don't actually understand how this ensures we actually build the `ppc/<builtin_name>.c` on powerpc/powerpc64/powerpc64le rather than some other one (for example `arm/<builtin_name>.c`).

This patch doesn't actually ensure this explicitly. The current version in master does but its very error prone because it does the wrong thing for powerpc and nobody noticed. This patch relies on everyone following the convention that the provided source files for a target are either the generic implementations or the implementations intended for that target. In my opinion this is reasonable and AFAICT this convention is already followed by the source code.
If this convention is broken I consider this to be a bug because it doesn't make sense to build a builtin library that contains both the arm and powerpc specific implementations for example.

Once the powerpc build is fixed I plan to revert `dc748816e2aec8941d63f8ad07fb82aff6be8af7` which will mean if someone breaks the convention it will actually cause testing to fail.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69189/new/

https://reviews.llvm.org/D69189