[PATCH] D35907: [WIP] Update TBAA information in stack coloring pass

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

We need to be very careful to document the contracts here and explain what's going on. The implementation of getUnderlyingObjects in ScheduleDAGInstrs.cpp calls its helper function getUnderlyingObjectFromInt which contains this comment:

  // If we find an add of a constant, a multiplied value, or a phi, it's
  // likely that the other operand will lead us to the base
  // object. We don't have to worry about the case where the
  // object address is somehow being computed by the multiply,
  // because our callers only care when the result is an
  // identifiable object.

and this is indeed true. The only existing caller, getUnderlyingObjectsForInstr, does this:

  } else if (const Value *V = MMO->getValue()) {
    SmallVector<Value *, 4> Objs;
    getUnderlyingObjects(V, Objs, DL);
  
    for (Value *V : Objs) {
      if (!isIdentifiedObject(V))
        return false;
  
      Objects.push_back(UnderlyingObjectsVector::value_type(V, true));
    }

and this is important because, if GetUnderlyingObjects hits its depth cutoff, it can return arbitrary instructions in the use/def chain of the pointer.

If we're going to make this function a utility that can be used elsewhere, we should make this safer. I'd advocate for moving the isIdentifiedObject check into the implementation of getUnderlyingObjects before:

  Objects.push_back(const_cast<Value *>(V));

such that if the check fails we clear the Objects array and return. That should make the interface safe. We should document this part of the functions behavior and remove the corresponding check in getUnderlyingObjectsForInstr.



================
Comment at: lib/CodeGen/ScheduleDAGInstrs.cpp:154
 /// ptrtoint+arithmetic+inttoptr sequences.
-static void getUnderlyingObjects(const Value *V,
-                                 SmallVectorImpl<Value *> &Objects,
-                                 const DataLayout &DL) {
+void llvm::getUnderlyingObjects(const Value *V,
+                          SmallVectorImpl<Value *> &Objects,
----------------
Please rename this function if you make it non-static. The function we have right now in ValueTracking is called GetUnderlyingObjects, but if we were to rename it follow our coding conventions we'd have a conflict with the name of function function. How about `getUnderlyingObjectsForCodeGen`?


================
Comment at: lib/CodeGen/StackColoring.cpp:917
+    // We keep both slots to maintain TBAA metadata later.
+    // FIXME: It is conservative. We can keep TBAA metadata
+    // if TBAA information of two slots match.
----------------
I'd remove this FIXME. It is not clear to me that there is a well-defined way to do this in general. A single alloca could be accessed at different times by accesses with incompatible TBAA metadata so long as some access compatible with both, or data dependence, keeps the correct ordering. As a result, there's not necessarily a single thing to merge the metadata with. I suppose merging everything (derived from one) with everything (derived from the other) might work, but in any event, it seems non-trivial. I'm happy to have someone revisit this if they find this needs to be more precise.


================
Comment at: lib/CodeGen/StackColoring.cpp:1005
+
+      // We adjust TBAA information for merged stack slots.
+      MachineSDNode::mmo_iterator MemOps =
----------------
This is not specific to TBAA. You should replace TBAA with AA metadata in all of the comments.


================
Comment at: lib/CodeGen/StackColoring.cpp:1014
+        // may share the same memory location.
+        bool MayModified = false;
+        if (MMO->getAAInfo().TBAA) {
----------------
I don't understand this name. How about `MayHaveConflictingAAMD`?


================
Comment at: lib/CodeGen/StackColoring.cpp:1034
+          // We eliminate information on TBAA from the existing AAInfo.
+          AAMDNodes NewAAInfo = AAMDNodes(nullptr, MMO->getAAInfo().Scope,
+                                          MMO->getAAInfo().NoAlias);
----------------
This situation is not specific to TBAA. If affects all of the AA metadata. You'll need to unset it all.


https://reviews.llvm.org/D35907