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

[Hiroshi Inoue via Phabricator via llvm-commits]

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


================
Comment at: include/llvm/CodeGen/MachineFunction.h:664
 
+  /// getMachineMemOperand - Allocate a new MachineMemOperand by copying
+  /// an existing one, replacing only AliasAnalysis information.
----------------
MatzeB wrote:
> Don't repeat the method name in the doxygen comment (old code does it, new code shouldn't).
Fixed.


================
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,
----------------
hfinkel wrote:
> 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`?
I renamed it getUnderlyingObjectsForCodeGen and moved into ValueTracking.cpp.


================
Comment at: lib/CodeGen/StackColoring.cpp:40
 #include "llvm/CodeGen/PseudoSourceValue.h"
+#include "llvm/CodeGen/ScheduleDAGInstrs.h" // for getUnderlyingObjects
 #include "llvm/CodeGen/SlotIndexes.h"
----------------
MatzeB wrote:
> Instead of documenting why you need to import a scheduling header here, just put the function into an apropriate header (as above) :)
Moved to ValueTracking.h.


================
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.
----------------
hfinkel wrote:
> 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.
I agree. I removed this FIXME.


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


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


================
Comment at: lib/CodeGen/StackColoring.cpp:1025
+                const AllocaInst *AI = dyn_cast_or_null<AllocaInst>(V);
+                if (AI && MergedAllocas.count(AI)) {
+                  MayModified = true;
----------------
MatzeB wrote:
> Is `MergedAllocas` changing here? Looks like you can move the check further up outside this loop.
We need to check each Value V (and hence AI) with MergedAllocas. So I do this check in the loop.


================
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);
----------------
hfinkel wrote:
> This situation is not specific to TBAA. If affects all of the AA metadata. You'll need to unset it all.
Done.


https://reviews.llvm.org/D35907