Page MenuHomePhabricator

[ValueTracking] Use call arguments for nonnull attribute check
Needs ReviewPublic

Authored by nikic on Jul 1 2021, 1:26 PM.

Details

Reviewers
spatel
Group Reviewers
Restricted Project
Summary

isKnownNonZero() can determine nonnull-ness from a dominating call with a nonnull parameter. Currently, this works by inspecting the attributes on the called function.

This code currently crashes with opaque pointers if the call-site and the called function have a different number of arguments (which no longer requires a bitcast to sit in between). Fix this by directly working with the call arguments instead.

As seen by changes on existing tests, this has the additional benefit that we now take into account attributes that are present only on the callsite, and not on the function declaration.

Unfortunately, this means we need to duplicate the function-level Attribute::hasNonNullAttr() logic into a call-level CallBase::paramIsNonNull() function.

Diff Detail

Event Timeline

nikic created this revision.Jul 1 2021, 1:26 PM
nikic requested review of this revision.Jul 1 2021, 1:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2021, 1:26 PM
dexonsmith added inline comments.Jul 1 2021, 4:22 PM
llvm/lib/IR/Instructions.cpp
325

I wonder if (some or most of) this logic could be factored out of here and Argument::hasNonNullAttr() if the common implementation took the Type and an AttributeList as parameters?

nikic added inline comments.Jul 2 2021, 12:06 PM
llvm/lib/IR/Instructions.cpp
325

I gave this a try: https://gist.github.com/nikic/d1ce3f7a7488be786974e040648a65e0

I'm not sure this really makes things better. Ultimately, the core logic here is simple, and moving it to AttributeSet means that we instead need to duplicate the "call or callee attributes" logic, so it ends up being bulkier overall.

are we going to allow mismatched argument types/num arguments for a direct call? it seems bad for usability and seems very easy to mess up frontends, or even simple handwritten IR tests
I'm aware that there are issues with this, it's sort of like [1], but we should probably highlight this as part of the opaque pointer transition

[1]: https://llvm.org/docs/FAQ.html#why-does-instcombine-simplifycfg-turn-a-call-to-a-function-with-a-mismatched-calling-convention-into-unreachable-why-not-make-the-verifier-reject-it

nikic added a comment.Jul 9 2021, 1:45 PM

are we going to allow mismatched argument types/num arguments for a direct call? it seems bad for usability and seems very easy to mess up frontends, or even simple handwritten IR tests
I'm aware that there are issues with this, it's sort of like [1], but we should probably highlight this as part of the opaque pointer transition

[1]: https://llvm.org/docs/FAQ.html#why-does-instcombine-simplifycfg-turn-a-call-to-a-function-with-a-mismatched-calling-convention-into-unreachable-why-not-make-the-verifier-reject-it

Good question. I think we do need to allow it for similar reasons. Currently, you can call a function with a different signature by inserting a bitcast between the function type pointers. With opaque pointers we no longer have that bitcast to indicate the signature mismatch.

With that in mind, I think that this patch goes the wrong way about fixing the original problem (though I still think the change makes sense for other reasons, like using call-site attributes). Namely, what getCalledFunction() currently does is simply check whether getCalledOperand() is a function. However, with opaque pointers we should additionally check that the call function type matches the declaration function type. Then call can continue under the reasonable assumption that the call and the declaration match. Effectively this means that we're going to treat a call to a function with a different signature as an indirect, mostly non-analyzable call, the same as we did before. If we don't do this, we break lots of code, especially code handling intrinsics.

This doesn't address the issue of unintentional mistakes. I don't think we can really do anything about that -- it's a general drawback of the opaque pointer approach that you no longer have the sanity check between value types and pointer element types.

Namely, what getCalledFunction() currently does is simply check whether getCalledOperand() is a function.

Duplicating slightly from my comment in https://reviews.llvm.org/D105733, scanning lib/Analysis it looks like many callers to expect it to see through bitcasts, like its header docs suggest it would. Note also that the AbstractCallSite version *does* look through bitcasts:

/// Return the function being called if this is a direct call, otherwise
/// return null (if it's an indirect call).
Function *getCalledFunction() const {
  Value *V = getCalledOperand();
  return V ? dyn_cast<Function>(V->stripPointerCasts()) : nullptr;
}