[PATCH] D82562: Implement AVX ABI Warning/error

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticFrontendKinds.td:246
+              "enabled changes the ABI">,
+      InGroup<DiagGroup<"avx-abi">>;
+def err_avx_calling_convention : Error<warn_avx_calling_convention.Text>;
----------------
Should this be -Wpsabi to match gcc? I think that's what gcc called it.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:4248
   const Decl *TargetDecl = Callee.getAbstractInfo().getCalleeDecl().getDecl();
-  if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(TargetDecl))
+  if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(TargetDecl)) {
     // We can only guarantee that a function is called from the correct
----------------
Wow that was a big comment for no curly's.


================
Comment at: clang/lib/CodeGen/CGCall.cpp:4259
 
+    // Some architectures (such as x86_64) have the ABI changed based on
+    // attribute-target/features. Give them a chance to diagnose.
----------------
nit should probably be x86-64. I think we only use underscore when we're already using - for something else like in the triple.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:2493
+                                 SourceLocation CallLoc,
+                                 llvm::StringMap<bool> &CallerMap,
+                                 llvm::StringMap<bool> &CalleeMap, QualType Ty,
----------------
Should these maps be const? Which means you have to use find to do the lookups.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:2554
+      if (checkAVXParam(CGM.getDiags(), CGM.getContext(), CallLoc, CallerMap,
+                        CalleeMap, Ty, /*isArgument*/ true))
+        return;
----------------
isArgument->IsArgument


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:2567
+                  CalleeMap, Callee->getReturnType(),
+                  /*isArgument*/ false);
+  }
----------------
Same here


Repository:
  rC Clang

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

https://reviews.llvm.org/D82562