[PATCH] D32586: [EarlyCSE] Teach EarlyCSE to work with non-instruction values

Philip Reames via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Apr 27 19:01:29 PDT 2017


reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:68
 
-  static bool canHandle(Instruction *Inst) {
+  Instruction *getInst() const {
+    assert(isInst() && "This SimpleValue does not represent an instruction!");
----------------
These three cover functions seem to obscure more than they help clarify.  Or at least they did for me.  I'd advice removing them and just directly working with the Val field.


================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:82
+  static bool canHandle(Value *Val) {
+    Instruction *Inst = dyn_cast<Instruction>(Val);
+    // We can handle all non-instruction values.
----------------
A tactical suggestion here: do the refactoring to change all the types w/o actually adding values to the canHandle predicate then separately check in the change which actually allows the Values to flow through.  (i.e. basically return false here in the initial patch)

The reasoning here is that if this does expose a problem (miscompile, crash, etc..), knowing whether you made a mistake in the refactoring part or if the value handling itself was the actual guilty party will be useful.

Not a required change, but something I would encourage.


================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:112
 unsigned DenseMapInfo<SimpleValue>::getHashValue(SimpleValue Val) {
-  Instruction *Inst = Val.Inst;
+  if (!Val.isInst())
+    return hash_combine(Val.getValue());
----------------
This would be more idiomatic as:
Instruction *Inst = dyn_cast<Instruction>(Val.Val);
if (!Inst)
  return hash_combine(Val.Val);


https://reviews.llvm.org/D32586





More information about the llvm-commits mailing list