[PATCH] Update optimization passes to handle inalloca arguments
Reid Kleckner
rnk at google.com
Fri Jan 17 17:21:54 PST 2014
A lot of this changed and I think the questionable parts went away. PTAL
================
Comment at: include/llvm/IR/Instructions.h:35
@@ -34,2 +34,3 @@
class LLVMContext;
+class ImmutableCallSite;
----------------
Nick Lewycky wrote:
> Sort.
Gone.
================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:84
@@ -83,1 +83,3 @@
+ // Inalloca arguments are considered to be clobbered by the call.
+ unsigned ArgNo = CS.getArgumentNo(UI);
----------------
Nick Lewycky wrote:
> "considered to be" --> "implicitly"? You get undef if you load from one afterwards, right?
Or just "are clobbered..."
================
Comment at: include/llvm/IR/Argument.h:64
@@ +63,3 @@
+ /// attribute on it in its containing function. These attributes are very
+ /// similar from the perspective of the callee.
+ bool hasByValOrInAllocaAttr() const;
----------------
Nick Lewycky wrote:
> "the byval attribute or inalloca attribute"
> or *the inalloca attribute?
>
> "from the perspective of the callee" doesn't really tell the reader much (why are they similar?). Suggestion: "These attributes both represent arguments being passed by copy under different ABIs."
Done.
================
Comment at: include/llvm/IR/Instructions.h:115
@@ +114,3 @@
+ /// isUsedWithInAlloca - Return true if this alloca is used as an inalloca
+ /// argument to a call. Such allocas are never considered static.
+ bool isUsedWithInAlloca() const;
----------------
Nick Lewycky wrote:
> Unclear what "static" means here.
It's unclear immediately after "isStaticAlloca()?"
================
Comment at: lib/IR/Instructions.cpp:935
@@ +934,3 @@
+ unsigned ArgNo = CS.getArgumentNo(UI);
+ if (CS.paramHasAttr(1 + ArgNo, Attribute::InAlloca))
+ return CS;
----------------
Nick Lewycky wrote:
> No action required: By the way, this is actually O(n^2) not O(n) because attributes are stored in a linked list so finding 1+ArgNo is not constant time. See PR16536.
Interesting. This code is gone now. =/
================
Comment at: include/llvm/IR/Instructions.h:120
@@ +119,3 @@
+ /// InAlloca attribute, or nothing. There can be zero or one such call sites.
+ ImmutableCallSite getInAllocaCallSite() const;
+
----------------
Nick Lewycky wrote:
> Optional: Why do you need isUsedWithInAlloca() instead of just calling this and testing the result? CallSite evaluates to false when zero-initialized.
Gone now, but I thought it read better.
http://llvm-reviews.chandlerc.com/D2449
More information about the llvm-commits
mailing list