This patch preserve dereferenceable, nonnull and alignment from loads and stores.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I love the progress :) Some comments below.
llvm/include/llvm/IR/KnowledgeRetention.h | ||
---|---|---|
35 ↗ | (On Diff #252521) | Feel free to commit this as NFC right away. |
llvm/lib/IR/KnowledgeRetention.cpp | ||
234 | Let's make this a member function like addCall. We should also merge this with existing logic and put it as a helper somewhere. Given an instruction, and potentially a set of attributes as filter, determine all attributes we can derive from the instruction. I might look into this as the attributor should use that as well to cut down on duplication. | |
238 | I'd switch on the opcodes, use if-else-if, or early returns. |
fixed comments.
llvm/lib/IR/KnowledgeRetention.cpp | ||
---|---|---|
234 | I agree we should try to prevent duplication but we will need to be careful not to be too restrictive about the representation the caller can use such that it can be used widely. an issue which is hopefully going to get resolved soon. i think this API should reside in Analysis and there are some issues with dependencies for KnowledgeRetention see D76149#inline-701084 for details. |
I only have coding style comments left. Providing the logic in a more general way will be future work.
llvm/lib/IR/KnowledgeRetention.cpp | ||
---|---|---|
189 | Nit: | |
200 | The above two lines is T, isn't it? | |
213 | Style: TBH, I'm not necessarily a fan of the overloading via lambda idea. Couldn't we pass all necessary information to addLoadOrStore. I mean, we make it addAccessedPtr(Instruction &Inst, Value &Ptr, Type &AccTy, MaybeAlign Alignment) |
One last question
llvm/lib/IR/KnowledgeRetention.cpp | ||
---|---|---|
202 | I wonder if we have to guard the nonnull stuff with DerefSize != 0. This is only needed if we have "no-op" 0-length accesses, I think. I doubt those exist. Wdyt? |
llvm/lib/IR/KnowledgeRetention.cpp | ||
---|---|---|
202 | i have not seen an example where DerefSize is 0 but since we obtain a minimum size it wouldn't surprise me if it did exist. |
LGTM
llvm/lib/IR/KnowledgeRetention.cpp | ||
---|---|---|
202 | Your right, it seems unlikely. Let's keep it like this. |
Nit:
We can have non pointer attributes (later).
-Ptr
+V or Val
(Then we'll also need to rename setPointer)