[PATCH] [GC] CodeGenPrep transform: simplify offsetable relocate
artagnon at gmail.com
Tue Jan 13 10:53:09 PST 2015
Expect a new iteration immediately.
Comment at: lib/CodeGen/CodeGenPrepare.cpp:541
@@ +540,3 @@
+// derived pointer relocation instructions given a vector of all relocate calls
+static void ComputeBaseDerivedRelocateMap(const SmallVector<User *, 2>
> llvm style is computeBase...
> Make the param SmallVectorImpl so that it's size independent.
I see `static bool SinkCast`, `static bool OptimizeNoopCopyExpression`, `static bool OptimizeCmpExpression`, but I suppose it's good to follow the documented convention when writing new functions? `simplifyOffsetableRelocate` it is.
Note: I can't do `SmallVectorImpl` inside the `DenseMap`.
Comment at: lib/CodeGen/CodeGenPrepare.cpp:594
@@ +593,3 @@
+// computes a replacement, and affects it.
+static void AffectRelocateReplacements(IntrinsicInst *GEPBase,
+ const SmallVector<IntrinsicInst *, 2>
> I think GEPBase might be more clearly named "RelocatedBase".
> Also, "affect" is a slightly odd verb. Possibly: SimplifyOneRelocate?
`SimplifyOneRelocate` is misleading because we're simplifying multiple derived-pointer relocates corresponding to the same base-pointer relocate. Perhaps `SimplifyRelocatesOffABase`?
Comment at: lib/CodeGen/CodeGenPrepare.cpp:616
@@ +615,3 @@
+ SmallVector<Value *, 2> OffsetV;
+ if (!GetGEPSmallConstantIntOffsetV(Derived, OffsetV))
> To more clearly seperate the legality and profitability, I might write this as:
> get gep indices
> visit gep indices, checking for constants
> Your current version might be slightly faster, but it's IMO less clear.
> Minor. I'll defer to your preference.
Get - GEP - Small - ConstantInteger - OffsetVector. I thought the name was clever and clear; my preference is not to change it.
Also, I found a bug: it shouldn't `return` if it couldn't find a small constant-integer offset-vector; it should `continue`, so that other derived pointer relocations get a chance.
Comment at: lib/CodeGen/CodeGenPrepare.cpp:624
@@ +623,3 @@
> I'm confused by the insertion placement games. Why isn't inserting the new GEP right after the relocation you're replacing okay?
Ha, this is fun. Consider:
%ptr = getelementptr i32* %base, i32 15
%tok = call i32 (i1 ()*, i32, i32, ...)* @llvm.experimental.gc.statepoint.p0f_i1f(i1 ()* @return_i1, i32 0, i32 0, i32 0, i32* %base, i32* %ptr)
%ptr-new = call i32* @llvm.experimental.gc.relocate.p0i32(i32 %tok, i32 4, i32 5)
%base-new = call i32* @llvm.experimental.gc.relocate.p0i32(i32 %tok, i32 4, i32 4)
%ret = load i32* %ptr-new
Here, if I replace the `%ptr-new` line with a `gep`, against whom am I gep'ing? `%base-new` ofcourse, but that's on the next line! The problem is therefore to reorder instructions so that the new instruction comes after `%base-new`, no matter what the case.
Comment at: lib/CodeGen/CodeGenPrepare.cpp:674
@@ +673,3 @@
+ for (auto &Item : RelocateInstMap)
+ AffectRelocateReplacements(Item.first, Item.second, MadeChange);
+ return MadeChange;
> Giving names to Item.first and Item.second would make this code more clear. Temporary variables could be used. Creating a helper struct rather than a pair would also be fine.
Hm? It's a pair because that's what you get when iterating over a `DenseMap`. I don't want to create new variables, so I've written comments about what `Item.first` and `Item.second` are, as a compromise.
More information about the llvm-commits