[PATCH] D64152: [Attributor] Deduce "align" attribute on return value

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:689
+      : AbstractAttribute(V, InfoCache) {}
+
+  /// See AbastractState::getAttrKind().
----------------
Please add getters for the assumed and known alignment here.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:816
+    }
+  }
+
----------------
Can you unify the two initialize methods in the base class (`AAAlignImpl`) with the index trick used in `addIfNotExistent`?



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:840
+  // of return value would't have align. (ii) There are two return values whose
+  // align value is not same.
+
----------------
What you want to do use to take the minimum of all align values and the current state (`IntegerState`). Remember the state at the beginning of this function to determine if it changed. We also need a test for this.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:852
+
+  std::function<bool(Value &)> Pred = [&](Value &RV) -> bool {
+    auto *AlignAA = A.getAAFor<AAAlignImpl>(*this, RV);
----------------
For the return value you should do two things (for now):
1) Use `Value::getPointerAlignment` to get the known alignment in the IR.
2) Use `getAAFor<AAAlign>()` to the the known and assumed alignment from the Attributor.

Later, 1) would be replaced by a new `ValueTracking.h` `getPointerAlignment` method that does traversal, e.g., look through casts, compute new alignment based on constant geps, etc. 
Later later, the new 1) and 2) would be merged to use traversal and assumed alignment together.

You can work on the later points as well if you want.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:872
+  if (AARetValImpl->checkForallReturnedValues(Pred) && AlignValue.hasValue()) {
+    indicateOptimisticFixpoint();
+    return ChangeStatus::CHANGED;
----------------
There is (probably) no need for the `ExistsAssume` trick here, you can leave it but add a comment to explain that it is a shortcut to identify an optimistic fixpoint.


================
Comment at: llvm/test/Transforms/FunctionAttrs/align.ll:22
+; TEST 3
+; ATTRIBUTOR: define i32* @test3(i32* align 8, i32* align 4, i1)
+define i32* @test3(i32* align 8, i32* align 4, i1) #0 {
----------------
We want `align 4` here.


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

https://reviews.llvm.org/D64152