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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:257
+    return Arg->getArgNo();
+  return 0;
 }
----------------
I think you want to `return -1` so it is clearly not a valid argument position.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:772
+
+    // Already the function has align attribute on return value.
+    if (F.getAttributes().hasAttribute(AttrIdx, Attribute::Alignment)) {
----------------
return value or argument, right?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:776
+          F.getAttribute(AttrIdx, Attribute::Alignment).getAlignment());
+      indicateOptimisticFixpoint();
+    }
----------------
One could use the existing attribute as "known" and not indicate a fixpoint so that we can compute a better value.
We should have a test for it even if you keep it this way to describe the possibility.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:786
+    if (getKnownAlign() <= (1U << 29)) {
+      Attrs.emplace_back(Attribute::getWithAlignment(Ctx, getKnownAlign()));
+    }
----------------
Why the if? Can't you take the minimum of the knownAlign and the maximum allowed? Or initialize the assumed to the maximum allowed value already in the initialization?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:787
+      Attrs.emplace_back(Attribute::getWithAlignment(Ctx, getKnownAlign()));
+    }
+  }
----------------
style: remove braces around single statements.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:838
+  // wouldn't have align. If there is no top state, we can reach optimistic
+  // fixpoint earlier.
+
----------------
> If there is no top state, we can reach optimistic fixpoint earlier.

"If no assumed state was used for reasoning, an optimistic fixpoint is reached earlier."


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:856
+      return true;
+    }
+
----------------
I think you want to first look at the in-flight `AAAlign` attribute before you look at the IR.


================
Comment at: llvm/test/Transforms/FunctionAttrs/align.ll:42
+; }
+
+
----------------
Can we have SCC tests as well


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

https://reviews.llvm.org/D64152