[PATCH] D31290: [x86] use PMOVMSK to replace memcmp libcalls for 16-byte equality

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6098
+  LHSVal = DAG.getBitcast(CmpVT, LHSVal);
+  RHSVal = DAG.getBitcast(CmpVT, RHSVal);
+
----------------
efriedma wrote:
> spatel wrote:
> > efriedma wrote:
> > > What's the point of performing the load in a vector type if you're going to immediately bitcast the result to an integer type?  IIRC DAGCombine will fold this away.
> > I actually had it loading i128 to start, but I saw 2 problems:
> > 1. The i128 loads+bitcasts weren't converted to vector loads directly. Legalization for x86-64 split this into i64 loads, and then we had to rely on the combiner to merge the loads. At the least, I think this would be slower to compile since it caused more nodes to be created and folded. At worst, we might not put the loads back together properly and that would lead to poor code. It wasn't clear to me that I could add a generic combine to do that either since some targets might not want that.
> > 2. It wasn't honest to use i128 loads and bypass the isTypeLegal() check. We could make the TLI hook more specialized to account for that - have it confirm that loads of a given type/size are fast, so it's truly just a memcmp hook. But given the first problem, I got scared away.
> If DAGCombine doesn't fold it away, this is fine, I guess.  Maybe let the target specify the type to use, in case some target wants to use a type that isn't `<4 x i32>`?
Yes - that would be better. We can cycle through the possible simple types (including i128), and the target can let us know what works.

Also, your example using "__int128_t" probably explains why we saw/expected different things after this step in the DAG. If the loads are aligned, then we will legalize these to v16i8 loads for an SSE2 target, but not if they are unaligned as I was seeing in my experiments.


https://reviews.llvm.org/D31290