[PATCH] D59744: Fix i386 ABI "__m64" type bug

[Wei Xiao via Phabricator via cfe-commits]

wxiao3 marked an inline comment as done.
wxiao3 added inline comments.


================
Comment at: lib/CodeGen/TargetInfo.cpp:919
 /// IsX86_MMXType - Return true if this is an MMX type.
 bool IsX86_MMXType(llvm::Type *IRType) {
-  // Return true if the type is an MMX type <2 x i32>, <4 x i16>, or <8 x i8>.
----------------
rnk wrote:
> wxiao3 wrote:
> > rnk wrote:
> > > I think looking at the LLVM type to decide how something should be passed is a bad pattern to follow. We should look at the clang AST to decide how things will be passed, not LLVM types. Would that be complicated? Are there aggregate types that end up getting passed directly in MMX registers?
> > For x86 32 bit target, no aggregate types end up getting passed in MMX register.
> > The only type passed by MMX is 
> > 
> > > __m64
> > 
> >  which is defined in header file (mmintrin.h):
> > 
> > 
> > ```
> > typedef long long __m64 __attribute__((__vector_size__(8), __aligned__(8)));
> > ```
> > 
> > Yes, it would be good if we define _m64 as a builtin type and handle it in AST level. But I'm afraid that it won't be a trivial work. Since GCC also handles __m64 in the same way as Clang currently does, can we just keep current implementation as it is?
> > 
> That's not quite what I'm suggesting. I'm saying that IsX86_MMXType should take a QualType parameter, and it should check if that qualtype looks like the __m64 vector type, instead of converting the QualType to llvm::Type and then checking if the llvm::Type is a 64-bit vector. Does that seem reasonable? See the code near the call site conditionalized on IsDarwinVectorABI which already has similar logic.
Yes, it's unnecessary to convert QualType to llvm::Type just for the _m64 vector type checking.
Since It's very simple to check _m64 type based on QualType with pre-conditioned type assertion 
```
if (const VectorType *VT = RetTy->getAs<VectorType>())
```
I just remove the utility function: IsX86_MMXType.


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

https://reviews.llvm.org/D59744