Page MenuHomePhabricator

[WIP] Adjust the users of dereferenceable wrt. dereferenceable_globally
Needs RevisionPublic

Authored by jdoerfert on Jun 12 2019, 11:02 PM.

Details

Summary

Prototype how we can make use of dereferenceable and
dereferenceable_globally at the same time. This depends on attributes
not in tree yet. This has not been tested nor have I updated the test
cases yet.

Event Timeline

jdoerfert created this revision.Jun 12 2019, 11:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 12 2019, 11:02 PM
reames added inline comments.Jun 13 2019, 9:31 AM
llvm/include/llvm/IR/Value.h
583

An API suggestion. Instead of adding a context pointer here, add a boolean OnlyAtDef out param. Then introduce a helper function in ValueTracking which does the inference over the IR. (i.e. leave this as returning basic facts about the value, and move the analysis into an analysis.)

Or if you want, call the param AssumeNoFree, or whatever..

jdoerfert marked 2 inline comments as done.

Update according to API suggestion

llvm/include/llvm/IR/Value.h
583

I like it, done. Though, I do need the location of the definition, so I made it an Instruction *& not a bool &, what do you think?

Update all uses of isDereferenceablePointer

courbet added inline comments.
llvm/lib/Transforms/Scalar/MergeICmps.cpp
165

I don't see where in the patch (or the rest of the stack of changes) the signature of isDereferenceablePointer is changed to non-const Instruction.

reames added inline comments.Jun 17 2019, 2:10 PM
llvm/include/llvm/IR/Value.h
583

I'd prefer the boolean given there's only two options: 1) on def, and 2) globally. I'm willing to defer to you on this if you feel strongly though.

jdoerfert marked 2 inline comments as done.Jun 18 2019, 1:18 AM
jdoerfert added inline comments.
llvm/include/llvm/IR/Value.h
583

My problem is that for option 1) "on def", the information that is returned would not be sufficient for most users. In the "on def" case you also need the "definition" to query maybeFreedInBetween. If I would return a boolean, I would need to implement a second traversal that does the same just to find the definition. Does this make sense?

llvm/lib/Transforms/Scalar/MergeICmps.cpp
165

True, I remove the const_cast. That was my bad.

reames added inline comments.Jun 18 2019, 8:10 AM
llvm/include/llvm/IR/Value.h
583

No, not really. You can only ask this question on a particularly value. (i.e. argument or return value or load) That is the definition. If you have the instruction to query the property, you have the def, so I really don't understand your point.

jdoerfert marked an inline comment as done.Jun 19 2019, 3:06 AM
jdoerfert added inline comments.
llvm/include/llvm/IR/Value.h
583

No, not really. You can only ask this question on a particularly value. (i.e. argument or return value or load) That is the definition. If you have the instruction to query the property, you have the def, so I really don't understand your point.

I'm also confused.

Let me try to explain again what I think we should do/need and why:

We call getPointerDereferenceableBytes on a pointer (llvm::Value) to get the "known dereferenceable bytes". This method determines that by traversing the operands until a "definition" is found. If the definition is natively dereferenceable, e.g., stack or global allocations, Instruction *&DerefKnownBefore is null and there is no change to the behavior we had. If the definition is "known dereferenceable" because of attributes or metadata that says _globally, Instruction *&DerefKnownBefore is null and there is no change to the behavior we had. Finally, if the definition is "known dereferenceable" because of attributes or metadata that does not end in _globally, Instruction *&DerefKnownBefore is set to the llvm:Instruction right after the "definition" with the attribute/metadata. This means that the number of dereferenceable bytes is known for that position but not necessarily any other position in the CFG. The caller can then use maybeFreedInBetween to determine if it holds for the position it is interested in. To have a tight "is potentially freed", one needs to know where it is "known dereferenceable" and where we are interested in "is dereferenceable".

reames requested changes to this revision.Jun 28 2019, 4:37 PM

Happy to talk offline (call, chat, etc..) since we seem to be talking past each other again.

llvm/lib/IR/Value.cpp
689

Same point as before, but explained differently in the hopes this makes sense.

DerefKnownBefore only ever takes one of two values: the first instruction within the function, or the one after the instruction we invoked this function on. As such, the return value adds no information which can't be easily determined from a single boolean.

This revision now requires changes to proceed.Jun 28 2019, 4:37 PM
jdoerfert marked an inline comment as done.Jul 1 2019, 1:52 PM
jdoerfert added inline comments.
llvm/lib/IR/Value.cpp
689

I think I get it know and I hope that means I can convince you we need more than a boolean, at least in the future and if we want to make it easier for the clients.

First, in the future we could handle any subset of the following cases which would make the boolean insufficient:

  • casts
  • select/phi
  • gep with constant offsets and dereferenceable base pointer
  • inbound geps with positive offsets w/ or w/o dereferenceabl base pointer

Second, the clients would need to do "more" in the boolean case as they have to check, cast, and advance the pointer value before they can call maybeFreedInbetween. Checked, means isa<Argument>(ptr) || isa<Instruction>(ptr) as we otherwise have an implicit contract that the bool is only set for instructions and arguments. Casted into the right thing, instruction or argument, and advanced in case of the former. I'd argue that will more easily to errors or wrong usage.

reames added inline comments.Jul 1 2019, 4:02 PM
llvm/lib/IR/Value.cpp
689

No, I'm not convinced.

I think what your describing is a need for two layers of APIs. There's the "raw" API on value which returns properties *of that value*, and then there's a more holistic API which returns *analysis results* and should live in Analysis/Something.h.

I'll also point out that future proofing code is a specific *anti-pattern* that we try very strongly to avoid. Choosing good abstractions is encouraged, but writing code which is not required by the current patch is not.

uenoku added a subscriber: uenoku.Jul 13 2019, 1:17 AM