[PATCH] D38479: Make -mgeneral-regs-only more like GCC's

[Renato Golin via Phabricator via cfe-commits]

rengolin added a comment.

In https://reviews.llvm.org/D38479#886587, @efriedma wrote:

> 1. We don't correctly ignore inline asm clobbers for registers which aren't allocatable (https://bugs.llvm.org/show_bug.cgi?id=30792)


This looks like a different (but related) issue. That should be fixed in the back-end, regardless of the new error messages.

> 2. We don't diagnose calls which need vector registers according to the C calling convention.

The function that checks for it returns true for vectors (`lib/Sema/SemaExprCXX.cpp:7487`). However, the tests cover floating point, but they don't cover vector calls (arm_neon.h).

> 3. We don't diagnose operations which have to be lowered to libcalls which need vector registers according to the C calling convention (fptosi, @llvm.sin.*, etc.).

Yup. That's bad.

> All three of these could be addressesed in the AArch64 backend in a straightforward manner.

I worry about declarations and front-end optimisations, which may move the line info to a completely different place (where it's used, for example). But I have no basis for that worry. :)

> Diagnosing floating-point operations in Sema in addition to whatever backend fixes we might want is fine, I guess, but I don't really like making "-mgeneral-regs-only" into "-mno-implicit-float" plus some diagnostics; other frontends don't benefit from this checking, and using no-implicit-float is asking for an obscure miscompile if IR generation or an optimization accidentally produces a floating-point value.

I agree with both statements. But it would be good to have the errors in Sema in addition to the back-end fixes, if there are cases where Clang's diagnostics is more precise on line info and carat position.



================
Comment at: lib/Sema/SemaExprCXX.cpp:7477
+static bool typeHasFloatingOrVectorComponent(
+    QualType Ty, llvm::SmallDenseMap<const RecordType *, bool, 8> &TypeCache) {
+  if (Ty.isNull())
----------------
The `TypeCache` object seems local.

It doesn't look like it needs to survive outside of this function, as per its usage and the comment:

    // We may see recursive types in broken code.

and it just adds another argument passing.


https://reviews.llvm.org/D38479