[PATCH] D27114: Preserve nonnull metadata on Loads through SROA & mem2reg.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Bunch of little comments below.

I'd like to see two high level thinsg before this goes in though:

1. I want to make sure Eli is happy with this approach w.r.t. any semantic differences between !nonnull metadata and the assume. Haven't seen an update from him since the switch of formulation.

2. I'd like at least a test case and a FIXME around the case where SROA rewrites the load and store to be integer loads and stores. This will happen fairly often due to things like unions, memcpy and other "bag of bits" interpretations of memory operations. We should work to not destroy !nonnull in the process by translating it to an assume. I'm happy for this to be handled in a subsequent patch, but I'd like to at least document the issue for anyone who ends up working on it.



================
Comment at: lib/Transforms/Utils/PromoteMemoryToRegister.cpp:41
 #include "llvm/Transforms/Utils/Local.h"
+#include "llvm/Transforms/Utils/PromoteMemToReg.h"
 #include <algorithm>
----------------
clang-format's header sort is wrong here (because PromoteMemToReg != PromoteMemoryToRegister).


================
Comment at: lib/Transforms/Utils/PromoteMemoryToRegister.cpp:306-313
+static void addAssumeNonNull(AssumptionCache *AC, LoadInst *LI) {
+    Function *AssumeIntrinsic = Intrinsic::getDeclaration(LI->getModule(), Intrinsic::assume);
+    ICmpInst *LoadNotNull = new ICmpInst(ICmpInst::ICMP_NE, LI, Constant::getNullValue(LI->getType()));
+    LoadNotNull->insertAfter(LI);
+    CallInst *CI = CallInst::Create(AssumeIntrinsic, {LoadNotNull});
+    CI->insertAfter(LoadNotNull);
+    AC->registerAssumption(CI);
----------------
This deosn't look like the right formatting... was clang-format misconfigured?


================
Comment at: lib/Transforms/Utils/PromoteMemoryToRegister.cpp:964-965
+      // it with an assume.
+      if (AC && LI->getMetadata(LLVMContext::MD_nonnull))
+          addAssumeNonNull(AC, LI);
+
----------------
Here as well.


================
Comment at: test/Transforms/SROA/preserve-nonnull.ll:6-7
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
----------------
Do you need these?


================
Comment at: test/Transforms/SROA/preserve-nonnull.ll:9
+
+; CHECK-LABEL: yummy_nonnull
+; CHECK: [[RETURN:%(.*)]] = load float*, float** %0, align 8
----------------
I would at least add the @ and maybe the define to this so that random other thinsg containing this string don't match confusingly down the road.


================
Comment at: test/Transforms/SROA/preserve-nonnull.ll:15
+
+define float* @yummy_nonnull(float**) {
+entry-block:
----------------
it's good to not use unnamed values in tests as they make future edits to the test brittle.


https://reviews.llvm.org/D27114