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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Almost there :) some comments below.



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:400
+
+      if (F->arg_size() != RFI.getNumArgs())
+        return false;
----------------
jdoerfert wrote:
> 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.
I think what is said here was not helpful. For the var arg case we have the function and a function type basically, both should be var arg and have the same number of arguments. I was thinking call vs function decl but that is not the case. My bad.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:413
+    return true;
+  }
+
----------------
Nit: Add a TODO at the beginning of this function reminding us that we should output information to the user (under debug output and via remarks).


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:464
+      });                                                                      \
+    }                                                                          \
   }
----------------
Nit: `= std::move(ArgsTypes)`


================
Comment at: llvm/test/Transforms/OpenMP/rtf_type_checking.ll:64
+;
+; There are two matches since the pass is run once per function.
----------------
hamax97 wrote:
> The pass is run as many times as functions exist in the file. Is that the expected behavior?
Yes, but part of the pass should not be run as often I guess. Though that is a problem for later.


================
Comment at: llvm/test/Transforms/OpenMP/rtf_type_checking.ll:2
+; RUN: opt -S -openmpopt -stats < %s 2>&1 | FileCheck %s
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
----------------
jdoerfert wrote:
> `; REQUIRES: asserts`
> 
> is needed so this works in builds that do not track statistics.
Nit: leave the RUN line at the top


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

https://reviews.llvm.org/D76058