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
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..
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?
True, I remove the const_cast. That was my bad.
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".
Happy to talk offline (call, chat, etc..) since we seem to be talking past each other again.
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.
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:
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.
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.