[PATCH] D24170: Refactor replaceDominatedUsesWith to have a flag to control whether to replace uses in BB itself.

[Chandler Carruth via llvm-commits]

chandlerc added inline comments.

================
Comment at: include/llvm/Transforms/Utils/Local.h:330-334
@@ -329,5 +329,7 @@
+
 /// Replace each use of 'From' with 'To' if that use is dominated by
-/// the end of the given BasicBlock. Returns the number of replacements made.
+/// the end of 'BB'. Returns the number of replacements made.
+/// Replace use of 'From' with 'To' in 'BB' if 'IncludeSelf' is true.
 unsigned replaceDominatedUsesWith(Value *From, Value *To, DominatorTree &DT,
-                                  const BasicBlock *BB);
+                                  const BasicBlock *BB, bool IncludeSelf);
 
----------------
This was also landed without review, but I actually think this API isn't the right one.

The first problem is that boolean flags to change state tend to make call sites really hard to read. All the reader sees is "false". Even if you add a comment with the name of the parameter, it doesn't help much because the parameter is pretty subtle. "IncludeSelf" is misleading as this isn't talking about instructions which use themselves, but rather users that happen to be in the same basic block.

This overall seems like a confusing way to describe the operation, because dominance is no longer a very precise term. I wonder if it would make more sense to just have the client do two separate operations where it first replaces uses dominated by the basic block and then has dedicated logic to handle the uses within the basic block with clear comments explaining why this works?

As one example of something that I'd want explanations around: what does it mean to replace an operand to a *phi node* in BB?

Anyways, not sure if the idea above has a really bad impact on the call sites (looots of code or something), or if you see other ways to structure the API that would be more clear?


https://reviews.llvm.org/D24170