[OpenMPOpt] Validate declaration types against the expected types

[hamax97]

Summary

Validation of the found runtime library functions declarations types (return and argument types) with the expected types.

[jdoerfert]

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.

llvm/lib/Transforms/IPO/OpenMPOpt.cpp

398

You can use pointer comparison on llvm::Type directly, they are unique.

400

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.

404

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 :)

413

Make this a (static) class function if possible.

[hamax97]

1. RFI is not created unless the declaration found matches the runtime function type.
2. Conditional modified to check when a function is variadic.
3. Comparison between Type* made directly.
4. The function that checks if the types matches was moved to a class static function.

[hamax97]

Added regression test that uses the statistics to show that "runtime functions" declarations with different types than the expected are not matched.

[jdoerfert]

You need to squash the commits locally into one or diff against the base or you only upload the latest ;)

[jdoerfert]

llvm/test/Transforms/OpenMP/rtf_type_checking.ll

2 ↗

(On Diff #250345)

; REQUIRES: asserts

is needed so this works in builds that do not track statistics.

47 ↗

(On Diff #250345)

Remove the attributes.

63 ↗

(On Diff #250345)

In the comment above you say the last runtime call is matched, maybe having one that is is actually a good thing.

[hamax97]

Regression test modified to match a proper use of a runtime library function.
PD: Sorry for all that mess before.

[hamax97]

llvm/test/Transforms/OpenMP/rtf_type_checking.ll

64 ↗

(On Diff #250426)

The pass is run as many times as functions exist in the file. Is that the expected behavior?

[jdoerfert]

Almost there :) some comments below.

llvm/lib/Transforms/IPO/OpenMPOpt.cpp

400

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.

413

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).

464

Nit: = std::move(ArgsTypes)

llvm/test/Transforms/OpenMP/rtf_type_checking.ll

64 ↗

(On Diff #250426)

Yes, but part of the pass should not be run as often I guess. Though that is a problem for later.

2 ↗

(On Diff #250345)

Nit: leave the RUN line at the top

[hamax97]

Minor change to test file + adding std::move to object copy.

[hamax97]

llvm/lib/Transforms/IPO/OpenMPOpt.cpp

400

I don't understand what you mean. You were thinking on comparing the function call against the declaration?. Because the optimizer does that for you. The idea was to compare the declaration against the real runtime function declaration, wasn't it?.

Then is this ok?, or what should I do?

[jdoerfert]

llvm/lib/Transforms/IPO/OpenMPOpt.cpp

400

I mean, add two tests for var-args functions. One where there are not enough arguments, one where there are too many. Both should be flagged as bad. That is, the size needs to match perfectly.

[hamax97]

llvm/lib/Transforms/IPO/OpenMPOpt.cpp

400

IMHO, I think adding a test case for when there are too many arguments in a variadic function wouldn't be useful since we can only compare against the arguments that are mandatory.

Check out how the number of arguments are being compared (Note that the argument types are checked only if the number of arguments is valid). There are 5 possible function declarations:

Variadic Function:

1. Same number of arguments -> Which is ok.
2. More arguments than expected. Which is ok, it's a variadic function.
3. Less arguments than expected. Which is NOT ok, and is covered by:

if (F->arg_size() < RTFArgTypes.size())

Non-variadic function:

1. Same number of arguments. Which is ok. What we want.
2. Different number of arguments. Which is NOT ok, and is covered by:

if (!IsVarArg && F->arg_size() > RTFArgTypes.size())

Together with the if statement above, that checks if has less arguments, covers the second case for non-variadic functions.

[jdoerfert]

llvm/lib/Transforms/IPO/OpenMPOpt.cpp

400

IMHO, I think adding a test case for when there are too many arguments in a variadic function wouldn't be useful since we can only compare against the arguments that are mandatory.

Sure, but we can (and should) change the way we check here so we can detect bad signatures of variadic functions that have too many or too few mandatory arguments.

More arguments than expected. Which is ok, it's a variadic function.

I disagree. We compare the declaration against an expected declaration, they should match. The number of optional arguments at the call sites is a different story though.

[hamax97]

Removing unnecessary check for variadic function.

[hamax97]

llvm/lib/Transforms/IPO/OpenMPOpt.cpp

400

Ohhh, sure, I got you. Sorry. I was confusing function call with function declaration. Now fixed. But I cannot have two function declarations with the same function name, opt complains, and the only variadic function inside OMPKinds.def is __kmpc_fork_call.

But I have a function declaration with fewer arguments than necessary which also covers the test case you are asking for. Or do you think I might need to create another test file?.

[jdoerfert]

LGTM. Thx!

llvm/lib/Transforms/IPO/OpenMPOpt.cpp

400

It's fine

[hamax97]

In D76058#1934950, @jdoerfert wrote:

LGTM. Thx!

Thanks for your time.
I don't have commit permissions. Does it have to pass kind of another check to be pushed?.

[pcc]

This change broke the msan buildbot: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/39755

==70358==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x5a602b2 in deleteParallelRegions /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Transforms/IPO/OpenMPOpt.cpp:127:9
    #1 0x5a602b2 in (anonymous namespace)::OpenMPOpt::run() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Transforms/IPO/OpenMPOpt.cpp:116:16
    #2 0x5a94252 in (anonymous namespace)::OpenMPOptLegacyPass::runOnSCC(llvm::CallGraphSCC&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Transforms/IPO/OpenMPOpt.cpp:559:19
    #3 0x3f53893 in RunPassOnSCC /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Analysis/CallGraphSCCPass.cpp:139:23

[jdoerfert]

In D76058#1937554, @pcc wrote:

This change broke the msan buildbot: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/39755

==70358==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x5a602b2 in deleteParallelRegions /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Transforms/IPO/OpenMPOpt.cpp:127:9
    #1 0x5a602b2 in (anonymous namespace)::OpenMPOpt::run() /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Transforms/IPO/OpenMPOpt.cpp:116:16
    #2 0x5a94252 in (anonymous namespace)::OpenMPOptLegacyPass::runOnSCC(llvm::CallGraphSCC&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Transforms/IPO/OpenMPOpt.cpp:559:19
    #3 0x3f53893 in RunPassOnSCC /b/sanitizer-x86_64-linux-fast/build/llvm-project/llvm/lib/Analysis/CallGraphSCCPass.cpp:139:23

Oversight, thanks! Should be fixed with f09f4b26762ab5d5c15ab6f66eb02a497c5f0b8e.

[thakis]

The fix breaks tests on Windows: http://45.33.8.238/win/11117/step_11.txt

llvm/lib/Transforms/IPO/OpenMPOpt.cpp

private:
   CallInst *getCallIfRegularCall(Value &V, RuntimeFunctionInfo *RFI = nullptr) {
     CallInst *CI = dyn_cast<CallInst>(&V);
     if (CI && !CI->hasOperandBundles() &&
         (!RFI || CI->getCalledFunction() == RFI->Declaration))
       return CI;
     return nullptr;
   }
 
-  /// Helper to initialize all runtime function information for those defined in
-  /// OpenMPKinds.def.
-  void initializeRuntimeFunctions() {
-    // Helper to check the types in the declaration against the expected types.
-    auto CheckTypes = [&](RuntimeFunctionInfo &RFI) {
-      if (!RFI.Declaration)
-        return true;
-
-      Function *F = RFI.Declaration;
-      if (F->getReturnType()->getTypeID() != RFI.ReturnType->getTypeID())
-        return false;
-
-      if (F->arg_size() != RFI.getNumArgs())
-        return false;
+  /// Returns true if the function declaration \p F matches the runtime
+  /// function types, that is, return type \p RTFRetType, and arguments types
+  /// \p RTFArgTypes. If the runtime function is variadic, \p IsVarArg, the
+  /// comparison is made until the runtime function number of arguments.
+  static bool declMatchesRTFTypes(Function *F, Type *RTFRetType, bool IsVarArg,
+                                  SmallVector<Type *, 8> &RTFArgTypes) {
+    if (!F)
+    if (F->getReturnType() != RTFRetType)
+    if (F->arg_size() < RTFArgTypes.size())
+      return false;
+    if (!IsVarArg && F->arg_size() > RTFArgTypes.size())
 
-      SmallVector<Type *, 8>::iterator RTFTyIt;
-      RTFTyIt = RFI.ArgumentTypes.begin();
-      for (Argument &Arg : F->args()) {
-        if (Arg.getType()->getTypeID() != (*RTFTyIt)->getTypeID())
-          return false;
+    auto RTFTyIt = RTFArgTypes.begin();
+      if (Arg.getType() != *RTFTyIt)
 
-        ++RTFTyIt;
-      }
+      if (IsVarArg && ++RTFTyIt == RTFArgTypes.end())
+        break;
 
-      return true;
-    };
+  }
 
+  /// Helper to initialize all runtime function information for those defined in
+  /// OpenMPKinds.def.
+  void initializeRuntimeFunctions() {
     // Helper to collect all uses of the decleration in the UsesMap.
     auto CollectUses = [&](RuntimeFunctionInfo &RFI) {
       unsigned NumUses = 0;
       if (!RFI.Declaration)
         return NumUses;
       OMPBuilder.addAttributes(RFI.Kind, *RFI.Declaration);
 
       NumOpenMPRuntimeFunctionsIdentified += 1;
auto CollectUses = [&](RuntimeFunctionInfo &RFI) {
           ++NumUses;
         }
       }
       return NumUses;
     };
 
 #define OMP_RTL(_Enum, _Name, _IsVarArg, _ReturnType, ...)                     \
   {                                                                            \
-    auto &RFI = RFIs[_Enum];                                                   \
-    RFI.Kind = _Enum;                                                          \
-    RFI.Name = _Name;                                                          \
-    RFI.IsVarArg = _IsVarArg;                                                  \
-    RFI.ReturnType = _ReturnType;                                              \
-    RFI.ArgumentTypes = SmallVector<Type *, 8>({__VA_ARGS__});                 \
-    RFI.Declaration = M.getFunction(_Name);                                    \
-    bool TypesMatch = CheckTypes(RFI);                                         \
-    (void)TypesMatch;                                                          \
-    unsigned NumUses = CollectUses(RFI);                                       \
-    (void)NumUses;                                                             \
-    LLVM_DEBUG({                                                               \
-      dbgs() << TAG << RFI.Name << (RFI.Declaration ? "" : " not")             \
-             << " found\n";                                                    \
-      if (RFI.Declaration)                                                     \
-        dbgs() << TAG << "-> got " << NumUses << " uses in "                   \
-               << RFI.UsesMap.size() << " different functions.\n";             \
-    });                                                                        \
+    SmallVector<Type *, 8> ArgsTypes({__VA_ARGS__});                           \
+    Function *F = M.getFunction(_Name);                                        \
+    if (declMatchesRTFTypes(F, _ReturnType, _IsVarArg, ArgsTypes)) {           \
+      auto &RFI = RFIs[_Enum];                                                 \
+      RFI.Kind = _Enum;                                                        \
+      RFI.Name = _Name;                                                        \
+      RFI.IsVarArg = _IsVarArg;                                                \
+      RFI.ReturnType = _ReturnType;                                            \
+      RFI.ArgumentTypes = ArgsTypes;                                           \
+      RFI.Declaration = F;                                                     \
+      unsigned NumUses = CollectUses(RFI);                                     \
+      (void)NumUses;                                                           \
+      LLVM_DEBUG({                                                             \
+        dbgs() << TAG << RFI.Name << (RFI.Declaration ? "" : " not")           \
+              << " found\n";                                                   \
+        if (RFI.Declaration)                                                   \
+          dbgs() << TAG << "-> got " << NumUses << " uses in "                 \
+                << RFI.UsesMap.size() << " different functions.\n";            \
+      });                                                                      \
+    }                                                                          \
   }
 #include "llvm/Frontend/OpenMP/OMPKinds.def"
 
     // TODO: We should attach the attributes defined in OMPKinds.def.
   }
 
   /// The underyling module.
   Module &M;