[PATCH] D35907: [StackColoring] Update AliasAnalysis information in stack coloring pass

[Hiroshi Inoue via Phabricator via llvm-commits]

inouehrs marked 2 inline comments as done.
inouehrs added inline comments.


================
Comment at: lib/Analysis/ValueTracking.cpp:3332
+      }
+      // If GetUnderlyingObjects fails to find an identifiable object,
+      // getUnderlyingObjectsForCodeGen also fails for safety.
----------------
Comments from Hal
> I don't think this check does that you want because, if V is a inttoptr, 
> this will always be false (i.e. the check for inttoptr will be dead).

> To preserve functionality, I believe you need the check here instead.
Yes, exactly. I moved the check with `isIdentifiedObject()` after check of `IntToPtr`.


================
Comment at: lib/CodeGen/StackColoring.cpp:1033-1034
+        if (MayHaveConflictingAAMD) {
+          MemOps[MemOpIdx++] = MF->getMachineMemOperand(MMO, AAMDNodes());
+          ReplaceMemOps = true;
+        }
----------------
MatzeB wrote:
> MatzeB wrote:
> > - Does `AAMDNodes()` mean it can alias with anything? In that case you could simply use `I.dropMemRefs()` as we make the same assumption for an instruction without any memory operands.
> > 
> > - Can't you simplify to `MemOps[MemOpIdx++]` to `*MMO = ...` and avoid the extra counter?
> Ignore my last point about the `MemOps`. I missed that fact that you are duplicating the information and not just replacing the existing operands (may help understand if you renamed the variable to `NewMemOps`).
- `AAMDNodes()` means no AA information and so optimizers think this instruction can alias with anything. I replace only AAInfo and other information in MMO (e.g. pointer, size etc) are still needed. So I eliminate only AAInfo.
- I renamed `MemOps` to `NewMemOps` for readability. Thank you for the suggestion.


https://reviews.llvm.org/D35907