[FuncSpec] Make sure function is actually the callee before trying to specialize.

[duck-37]

Summary

This causes issues if we start allowing functions with their addresses taken to be specialized.
I intend to submit a patch that does this soon, so this is a blocker at the moment.

[SjoerdMeijer]

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

665

Hmmm, interesting. Can this lead to a miscompute, or other problems? I think we need to add a test to trigger that (which this patch fixes)?

[duck-37]

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

665

Yes, this can lead to miscompiles and crashes if not handled properly. The problem is that I *believe* this issue only arises in very specific scenarios and only when we start allowing specialization of functions that can get their address taken. Otherwise this is never triggered on problematic functions.

Since allowing that is outside of the scope of this PR, I thought it would be best to add the test for that in a separate patch I'm planning on submitting later.

[SjoerdMeijer]

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

665

I don't think we can or should submit code that isn't covered by tests, so I think that leaves us 2 options here:

• if we can't trigger this now, we could turn this into an assert.
• if we can trigger the problematic behaviour in a later patch, then it should be be part of that (and covered with tests there).

These options are not mutually exclusive, we could add an assert now, change that later?

[SjoerdMeijer]

Thanks for contributing to function specialisation by the way, I have also noticed your other patch that I will look at soon. I am happy to commit things on your behalf like the previous one, but you have a few patches in the pipeline and if you plan to do more llvm work, you could consider requesting an account so that you can commit it yourself. I believe the process to get commit rights isn't that difficult and should be quick. But up to you, like I said, am happy to commit things on your behalf.

[duck-37]

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

665

SGTM.

[duck-37]

In D111660#3060585, @SjoerdMeijer wrote:

Thanks for contributing to function specialisation by the way, I have also noticed your other patch that I will look at soon. I am happy to commit things on your behalf like the previous one, but you have a few patches in the pipeline and if you plan to do more llvm work, you could consider requesting an account so that you can commit it yourself. I believe the process to get commit rights isn't that difficult and should be quick. But up to you, like I said, am happy to commit things on your behalf.

I'll look into it. Thanks!

[duck-37]

Change from if statement to assert.

[SjoerdMeijer]

Thanks, LGTM.
(and let me know if I need to commit this on your behalf, but sounds like you'll be looking into getting an account)

[SjoerdMeijer]

Oh wait, sorry, regressions test fail, they trigger this assert. We should assert that CS.getCalledFunction() == F? Also, in asserts it is custom to add a message/diagnostic, so that will be:

assert(CS.getCalledFunction() == F && "function and callee should be same");

or something along those lines.

[duck-37]

In D111660#3061173, @SjoerdMeijer wrote:

Oh wait, sorry, regressions test fail, they trigger this assert. We should assert that CS.getCalledFunction() == F? Also, in asserts it is custom to add a message/diagnostic, so that will be:

assert(CS.getCalledFunction() == F && "function and callee should be same");

or something along those lines.

You're absolutely right, I'm running on not that much sleep at the moment, sorry!

[duck-37]

Fix faulty assertion.

[SjoerdMeijer]

Yep, cheers, LGTM

[duck-37]

Make clang-format happy.

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

bool getPossibleConstants(Argument *A,
     Function *F = A->getParent();
     bool AllConstant = true;
 
     // Iterate over all the call sites of the argument's parent function.
     for (User *U : F->users()) {
       if (!isa<CallInst>(U) && !isa<InvokeInst>(U))
         continue;
       auto &CS = *cast<CallBase>(U);
+
+      // This should not happen at the moment; however, it may if we start
+      // specializing functions that have their addresses taken.
+      assert(CS.getCalledFunction() == F && 
+             "Function and callee should be same!");
+
       // If the call site has attribute minsize set, that callsite won't be
       // specialized.
       if (CS.hasFnAttr(Attribute::MinSize)) {
         AllConstant = false;
         continue;
       }
 
       // If the parent of the call site will never be executed, we don't need