[PATCH] D29781: Add alias canonicalization pass when preparing for ThinLTO

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: lib/Transforms/Utils/CanonicalizeAliases.cpp:35
+  if (BitCastOperator *BC = dyn_cast<BitCastOperator>(V)) {
+    for (Use &U : BC->materialized_uses()) {
+      return userIsAlias(U.getUser());
----------------
mehdi_amini wrote:
> Why `materialized_uses`?
IIRC this was required when I was doing via a bitcode upgrade, because uses() asserts that the module is materialized and I believe it isn't on some paths (maybe when we are importing and the whole module isn't materialized?). It isn't strictly necessary with this patch, but will probably need to go back in when I add a follow on bitcode upgrade interface. I will remove it for now though.


================
Comment at: lib/Transforms/Utils/CanonicalizeAliases.cpp:36
+    for (Use &U : BC->materialized_uses()) {
+      return userIsAlias(U.getUser());
+    }
----------------
mehdi_amini wrote:
> That deserves a comment. I'd even desugarize the loop with an assert that there is only one use as "documentation" of the invariant you're relying on here. (alternatively, or at the same time, add a comment)
Will do


================
Comment at: lib/Transforms/Utils/CanonicalizeAliases.cpp:99
+    // RAUW all uses of original aliasee with the new alias.
+    replaceAllUsesWithExceptAliases(GO, NewGA);
+
----------------
mehdi_amini wrote:
> Instead of reimplementing a custom RAUW (the duplication isn't great here), would it be possible to perform the regular one, and then reattach the aliases?
> 
Hmm, maybe. Will take a stab at it.


================
Comment at: lib/Transforms/Utils/CanonicalizeAliases.cpp:110
+// Legacy pass that canonicalizes aliases.
+class CanonicalizeAliasesLegacyPass : public ModulePass {
+
----------------
mehdi_amini wrote:
> New PM version of the pass?
Good idea! Will add


https://reviews.llvm.org/D29781