[PATCH] D76058: Validate declaration types against the expected types

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Thanks for the patch!

(Check out the phabricator documentation on llvm.org to see how to upload patches with full context)

Two things that are missing are:

1. clear the RFI, or not create it in the first place if the types not match, e.g., use `_ReturnType` and `__VA_ARGS__` directly.
2. Add test cases for the different problems, too many/few arguments, wrong return or argument type. You can for example test it via the statistics or by verifying an optimization does not happen, e.g., deduplication.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:398
+      if (F->getReturnType()->getTypeID() != RFI.ReturnType->getTypeID())
+        return false;
+
----------------
You can use pointer comparison on `llvm::Type` directly, they are unique.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:400
+
+      if (F->arg_size() != RFI.getNumArgs())
+        return false;
----------------
You also need to make sure if the function is varidic as we might expect less arguments. There should be a test as well, we already have known functions that are variadic `__kmpc_fork_call` or something similar.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:404
+      SmallVector<Type *, 8>::iterator RTFTyIt;
+      RTFTyIt = RFI.ArgumentTypes.begin();
+      for (Argument &Arg : F->args()) {
----------------
This is a good case for `auto` (IMHO). The kind of container is an implementation detail that should not escape and is not helpful, `auto RTFTyIt = ...;` should do the trick :)


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:413
+      return true;
+    }; 
+
----------------
Make this a (static) class function if possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76058