[PATCH] [GC] CodeGenPrep transform: simplify offsetable relocate

[Philip Reames]

Response to comments, not yet looking at new version.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:310
@@ +309,3 @@
+      for (Instruction &I : BB)
+	if (isStatepoint(I))
+	  MadeChange |= SimplifyOffsetableRelocate(I);
----------------
artagnon wrote:
> reames wrote:
> > You've got a whitespace problem here.  Please fix indentation.
> > 
> > Also, you may be invalidating the iterator you're using here.  I'd strongly recommend collecting the statepoints into a SmallVector than processing them.  (i.e. two pass)
> Whitespace: Nobody is maintaining the Emacs modes in-tree, so I'm using my own. I forgot about indent-tabs-mode.
> 
> SmallVector done. Why will the iterator get invalidated though? I'm not modifying the instruction in `SimplifyOffsetableRelocate`, am I?
I did say *may*.  After reading through your code, it was actually fine, but for clarity, I might do the two pass anyways.  Your choice.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:313
@@ +312,3 @@
+
+    if (MadeChange)
+      ModifiedDT = true;
----------------
artagnon wrote:
> reames wrote:
> > These changes should change the dom tree?  Why is this needed?
> Removed.
> 
> I thought I needed this because the dominator tree is changing (when we reorder the base relocate to come before the derived relocate)?
Replacing instructions does not modify the cached dominator tree analysis.

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:617
@@ +616,3 @@
+      auto Derived = dyn_cast<GetElementPtrInst>(ThisRelocate.derivedPtr());
+      if (Derived && Derived->getPointerOperand() == Base) {
+	SmallVector<Value *, 2> OffsetV;
----------------
artagnon wrote:
> reames wrote:
> > reames wrote:
> > > Indentation!  Are you using tabs?
> > Early continue please, or if (auto Derived = ...) at least.
> > 
> > You need to break up this function for readability.  If you want specific suggestions, ask, but any reasonable division should do.  
> Not sure what you mean: you want a `if (!Derived) continue;` so the user doesn't have to read till the end of the block to figure out that it's continuing?
Less "read till the end" and more "not have to keep track of another nesting level"

================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:626
@@ +625,3 @@
+	cast<Instruction>(Repl)->removeFromParent();
+	cast<Instruction>(Repl)->insertAfter(GEPBase);
+	Repl->takeName(Target);
----------------
artagnon wrote:
> reames wrote:
> > p.s.  By this point in the code, I've completely lost track of what Target and Repl refer to.  You need better variable names and to break this function up.
> If my variable names are too long, I hit the 80-column limit; If they're too short, they aren't descriptive enough :|
> 
> I'll figure something out.
That is the constant tension. :)

http://reviews.llvm.org/D6883

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/