[PATCH] D66967: [Attributor] ValueSimplify Abstract Attribute

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I like it! First comments inlined.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2390
+    return true;
+  }
+
----------------
I guess you need to track dependences here somewhere.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2447
+    // If everyting is clear, reach optimistic fixpoint.
+    return ExistNone ? ChangeStatus::UNCHANGED : indicateOptimisticFixpoint();
+  }
----------------
Why unchanged? Don't we have to indicate *any* change in the state? (Same below)


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2457
+    if (!getAssociatedFunction())
+      indicatePessimisticFixpoint();
+  }
----------------
The associated function needs to have an exact definition as well.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2477
+    return ExistNone ? ChangeStatus::UNCHANGED : indicateOptimisticFixpoint();
+  }
+};
----------------
There is a version that omits the `ReturnInst` vector if you don't need it.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2496
+    // TODO: Implement this for phi, GEP, and etc.
+    return indicatePessimisticFixpoint();
+  }
----------------
`GenericValueTraversal` time ;)


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2500
+using AAValueSimplifyCallSiteReturned = AAValueSimplifyReturned;
+using AAValueSimplifyCallSiteArgument = AAValueSimplifyFloating;
+
----------------
I know I started this habit but I now think it is a bad idea to shortcut the classes with `using`. For one, we don't have nice statistics. Also, the class can derive from the other one and it won't be much more code.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:3281
 CREATE_VALUE_ABSTRACT_ATTRIBUTE_FOR_POSITION(AAAlign)
+CREATE_VALUE_ABSTRACT_ATTRIBUTE_FOR_POSITION(AAValueSimplify)
 
----------------
I think we want this to exists for *all* position kinds.

Think of an indirect call that takes a function pointer. We see a function pointer phi, which resolves to different functions, which we cannot simplify further but we should be able to handle it.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66967/new/

https://reviews.llvm.org/D66967