[PATCH] D37216: [SROA] propagate !range metadata when moving loads

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: lib/Transforms/Utils/Local.cpp:1893
+    NewLI.setMetadata(LLVMContext::MD_range, N);
+  }
 
----------------
return here.


================
Comment at: lib/Transforms/Utils/PromoteMemoryToRegister.cpp:315
+{
+  if (!AC) return; // FIXME: cargo cult.
+  if (LI->getMetadata(LLVMContext::MD_nonnull) &&
----------------
Huh?


================
Comment at: lib/Transforms/Utils/PromoteMemoryToRegister.cpp:318
+      !llvm::isKnownNonNullAt(ReplVal, LI, &DT))
+  {
+    addAssumeNonNull(AC, LI);
----------------
Brace on previous line.


================
Comment at: lib/Transforms/Utils/PromoteMemoryToRegister.cpp:328
+      DEBUG(dbgs() << "trying to move !range metadata from" <<
+	     *LI << " to" << *NewLI << "\n");
+      if (isValidAssumeForContext(LI, NewLI, &DT)) {
----------------
Two space indent.


================
Comment at: lib/Transforms/Utils/PromoteMemoryToRegister.cpp:329
+	     *LI << " to" << *NewLI << "\n");
+      if (isValidAssumeForContext(LI, NewLI, &DT)) {
+	copyRangeMetadata(DL, *LI, N, *NewLI);
----------------
We should refactor this (calling this function here seems odd - the load is not an assume). I think that you're trying to reuse the logic here for the dominance checking, which is fine, but you don't need the isEphemeralValueOf check). Please feel free to break isValidAssumeForContext into two parts, and reuse here the first part.


================
Comment at: lib/Transforms/Utils/PromoteMemoryToRegister.cpp:330
+      if (isValidAssumeForContext(LI, NewLI, &DT)) {
+	copyRangeMetadata(DL, *LI, N, *NewLI);
+      }
----------------
Odd indentation.


https://reviews.llvm.org/D37216