[PATCH] D10555: [X86] Replace avx2.pbroadcast intrinsics with native IR.

[Sanjay Patel via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

In http://reviews.llvm.org/D10555#226023, @RKSimon wrote:

> In http://reviews.llvm.org/D10555#225988, @chandlerc wrote:
>
> > FWIW, I really like this patch. Is there anything we can do to make this work?
>
>
> It appears we have a few things that need to be decided before going any further:
>
> 1 - When is it permitable to replace a (sub)target-specific intrinsic with a non-specific implementation in the headers (e.g. using __builtin_shufflevector for these broadcasts)?
>
> As long as the expected instruction remains in debug code I'm keen for this to be encouraged - we can add suitable tests, remove those builtin intrinsics to AutoUpgrade.cpp until 4.0 and get much cleaner headers.
>
> 2 - When is it permitable to replace a (sub)target-specific intrinsic in IR/DAG creation, and should that occur in InstCombine or in the target ISel code someplace?
>
> I'd vote for InstCombine as we already appear to have a critical mass of intrinsics here.
>
> 3 - What are we going to do to fix the issue introduced by the header refactor removing the target guards, causing a tricky to decipher 'Cannot select: intrinsic %llvm.x86.vcvtps2ph.128' style backend error for intrinsics that are implemented as macros?
>
> A quick+nasty solution would be to add header guards at least around each of those macros.


At some point while working on one of these, Andrea told me about one more place where we do some builtin/intrinsic handling. I think that was CGBuiltin.cpp in clang's CodeGen. It seems messy that we have at least 3 ways of dealing with these things, but there are probably good reasons for each.

I don't want to hold up progress, so I don't object to this patch going in as-is (especially since Simon confirmed that -O0 code looks fine for these cases). But it would be great to answer the design questions that Simon has raised here for our collective future reference. There will surely be more intrinsics where these came from. :)


http://reviews.llvm.org/D10555