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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

LGTM. Thx!



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:400
+
+      if (F->arg_size() != RFI.getNumArgs())
+        return false;
----------------
hamax97 wrote:
> jdoerfert wrote:
> > hamax97 wrote:
> > > jdoerfert wrote:
> > > > hamax97 wrote:
> > > > > jdoerfert wrote:
> > > > > > 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.
> > > > > 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?
> > > > 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.
> > > 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.
> > > 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.
> 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?.
> 
> 
It's fine


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

https://reviews.llvm.org/D76058