Eliminate instances of `EmitScalarExpr(E->getArg(n))` in EmitX86BuiltinExpr().

[thakis]

Summary

EmitX86BuiltinExpr() emits all args into Ops at the beginning, so don't do that work again.

This changes behavior: If e.g. ++a was passed as an arg, we incremented a twice previously. This change fixes that bug.

[rsmith]

I don't think this is NFC. Testcase:

long long int a, b, c, d;
unsigned char f() { return _InterlockedCompareExchange128(&(++a), ++b, ++c, &(++d)); }

Today, Clang increments a, b, c, and d twice each in f().

[thakis]

In D50979#1206211, @rsmith wrote:

I don't think this is NFC. Testcase:

long long int a, b, c, d;
unsigned char f() { return _InterlockedCompareExchange128(&(++a), ++b, ++c, &(++d)); }

Today, Clang increments a, b, c, and d twice each in f().

Thanks for pointing this out, good to hear that this even happens to find a bug :-) I'll add some tests to document the progression.

[thakis]

In D50979#1206282, @thakis wrote:

In D50979#1206211, @rsmith wrote:

I don't think this is NFC. Testcase:

long long int a, b, c, d;
unsigned char f() { return _InterlockedCompareExchange128(&(++a), ++b, ++c, &(++d)); }

Today, Clang increments a, b, c, and d twice each in f().

Thanks for pointing this out, good to hear that this even happens to find a bug :-) I'll add some tests to document the progression.

*happens to _fix_ a bug

[t.p.northover]

EmitAArch64BuiltinExpr() also emits args into Ops before the big switch (with some more subtlety around the last arg that I don't understand), but then almost every switch case does EmitScalarExpr(E->getArg(n)).

Took me a while to remember, but I can at least explain this bit now I think. The key is that there are two kinds of NEON intrinsics. Overloaded ones have a constant last argument that describes the real type, and non-overloaded ones use that last argument as a normal parameter.

The first massive switch you see handles the ones in the second case, so it always CodeGens the last parameter, but if you scroll all the way down to line 7369 there's another switch where only the pregenerated Ops are used, and this last arg is visible as Ty and/or VTy.

It may or may not be the best way to handle that situation, of course.

[thakis]

Add tests.

[hans]

lgtm

[thakis]

r340348, thanks!

[rnk]

clang/lib/CodeGen/CGBuiltin.cpp

10471

I noticed that EmitMSVCBuiltinExpr evaluates args again, so each one of these intrinsic implementations has this same bug. :(

I'll put together a fix.

It's a bit not straightforward because AArch64 doesn't pre-evaluate all the builtin arguments like x86, so we need an adapter from one style to the other.

clang/lib/CodeGen/CGBuiltin.cpp

case X86::BI_InterlockedExchangeSub64:
     return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedExchangeSub, E);
   case X86::BI_InterlockedOr64:
     return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedOr, E);
   case X86::BI_InterlockedXor64:
     return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedXor, E);
   case X86::BI_InterlockedDecrement64:
     return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedDecrement, E);
   case X86::BI_InterlockedIncrement64:
     return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedIncrement, E);
   case X86::BI_InterlockedCompareExchange128: {
     // InterlockedCompareExchange128 doesn't directly refer to 128bit ints,
     // instead it takes pointers to 64bit ints for Destination and
     // ComparandResult, and exchange is taken as two 64bit ints (high & low).
     // The previous value is written to ComparandResult, and success is
     // returned.
 
     llvm::Type *Int128Ty = Builder.getInt128Ty();
     llvm::Type *Int128PtrTy = Int128Ty->getPointerTo();
 
     Value *Destination =
-        Builder.CreateBitCast(EmitScalarExpr(E->getArg(0)), Int128PtrTy);
-    Value *ExchangeHigh128 =
-        Builder.CreateZExt(EmitScalarExpr(E->getArg(1)), Int128Ty);
-    Value *ExchangeLow128 =
-        Builder.CreateZExt(EmitScalarExpr(E->getArg(2)), Int128Ty);
-    Address ComparandResult(
-        Builder.CreateBitCast(EmitScalarExpr(E->getArg(3)), Int128PtrTy),
+        Builder.CreateBitCast(Ops[0], Int128PtrTy);
+    Value *ExchangeHigh128 = Builder.CreateZExt(Ops[1], Int128Ty);
+    Value *ExchangeLow128 = Builder.CreateZExt(Ops[2], Int128Ty);
+    Address ComparandResult(Builder.CreateBitCast(Ops[3], Int128PtrTy),
                             getContext().toCharUnitsFromBits(128));
 
     Value *Exchange = Builder.CreateOr(
         Builder.CreateShl(ExchangeHigh128, 64, "", false, false),
         ExchangeLow128);
 
     Value *Comparand = Builder.CreateLoad(ComparandResult);
 
     AtomicCmpXchgInst *CXI =
case X86::BI__int2c: {
     CS.setAttributes(NoReturnAttr);
     return CS.getInstruction();
   }
   case X86::BI__readfsbyte:
   case X86::BI__readfsword:
   case X86::BI__readfsdword:
   case X86::BI__readfsqword: {
     llvm::Type *IntTy = ConvertType(E->getType());
-    Value *Ptr = Builder.CreateIntToPtr(EmitScalarExpr(E->getArg(0)),
-                                        llvm::PointerType::get(IntTy, 257));
+    Value *Ptr =
+        Builder.CreateIntToPtr(Ops[0], llvm::PointerType::get(IntTy, 257));
     LoadInst *Load = Builder.CreateAlignedLoad(
         IntTy, Ptr, getContext().getTypeAlignInChars(E->getType()));
     Load->setVolatile(true);
     return Load;
   }
   case X86::BI__readgsbyte:
   case X86::BI__readgsword:
   case X86::BI__readgsdword:
   case X86::BI__readgsqword: {
     llvm::Type *IntTy = ConvertType(E->getType());
-    Value *Ptr = Builder.CreateIntToPtr(EmitScalarExpr(E->getArg(0)),
-                                        llvm::PointerType::get(IntTy, 256));
+    Value *Ptr =
+        Builder.CreateIntToPtr(Ops[0], llvm::PointerType::get(IntTy, 256));
     LoadInst *Load = Builder.CreateAlignedLoad(
         IntTy, Ptr, getContext().getTypeAlignInChars(E->getType()));
     Load->setVolatile(true);
     return Load;
   }
   case X86::BI__builtin_ia32_paddusb512:
   case X86::BI__builtin_ia32_paddusw512:
   case X86::BI__builtin_ia32_paddusb256: