[PATCH] D90329: [PowerPC] Fix va_arg in C++, Objective-C on 32-bit ELF targets

[George Koehler via Phabricator via cfe-commits]

kernigh added a comment.

I forgot about this diff for a month.



================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:4722
   bool isI64 = Ty->isIntegerType() && getContext().getTypeSize(Ty) == 64;
-  bool isInt =
-      Ty->isIntegerType() || Ty->isPointerType() || Ty->isAggregateType();
+  bool isInt = !Ty->isFloatingType();
   bool isF64 = Ty->isFloatingType() && getContext().getTypeSize(Ty) == 64;
----------------
nemanjai wrote:
> nit (feel free to ignore): seems like a better name might be something like `isSingleGPR` since it would appear that this is for types that go into a (single) general purpose register.
Hi. `isInt` is also true for a 64-bit integer, which goes into a pair of GPRs, because each register has 32 bits.


================
Comment at: clang/test/CodeGenCXX/ppc32-varargs-method.cpp:15
+// CHECK-NEXT: load i8*, i8** %{{[0-9]+}}, align 4
+// CHECK-NEXT: mul i8 %numUsedRegs, 4
+// CHECK-NEXT: getelementptr inbounds i8, i8* %{{[0-9]+}}, i8 %{{[0-9]+}}
----------------
efriedma wrote:
> Not sure referring to numUsedRegs like this will work in a non-Asserts build.  Please verify.
I did a build with cmake -DLLVM_ENABLE_ASSERTIONS=OFF and verified that the ppc32*varargs* tests still pass. `%numUsedRegs` is still in the llvm output.

`%numUsedRegs` //does not// appear in the output of `clang -S -emit-llvm ...` (not cc1), but `%numUsedRegs` //does// appear in the output of `clang -cc1 ...`. The test runs cc1 and sees the `%numUsedRegs`. I am confused by how llvm sometimes erases the name of `%numUsedRegs` and sometimes keeps the name, but I observe that turning LLVM_ENABLE_ASSERTIONS on or off doesn't affect the test.


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

https://reviews.llvm.org/D90329