[CGP] Fix handling of null pointer values in optimizeMemoryInst

[john.brawn]

Summary

The current way that trivial addressing modes are detected incorrectly thinks that null pointers are non-trivial, leading to an infinite loop where we keep duplicating the same select. Fix this by being aware of null when deciding if an addressing mode is trivial.

[skatkov]

Agreed. I also came up to the same case (null is not considered as non-trivial) however not sure it is a cause of hangs-up due to select is off by default in LLVM trunk at this moment. But probably we have the same issue with Phi.

There is also one issue related to initialMap due to it seems it is possible that two bases can have different types but we crash with assert in this case... Will try to come up with a patch today.

llvm/trunk/lib/CodeGen/CodeGenPrepare.cpp

if (Scale && other.Scale && Scale != other.Scale)
       Result |= ScaleField;
 
     if (countPopulation(Result) > 1)
       return MultipleFields;
     else
       return static_cast<FieldName>(Result);
   }
 
-  // AddrModes with a baseReg or gv where the reg/gv is
-  // the only populated field are trivial.
+  // An AddrMode is trivial if it involves no calculation i.e. it is just a base
+  // with no offset.
   bool isTrivial() {
-    if (BaseGV && !BaseOffs && !Scale && !BaseReg)
-      return true;
-
-    if (!BaseGV && !BaseOffs && !Scale && BaseReg)
-      return true;
-
-    return false;
+    // An AddrMode is (BaseGV + BaseReg + BaseOffs + ScaleReg * Scale) so it is
+    // trivial if at most one of these terms is nonzero, except that BaseGV and
+    // BaseReg both being zero actually means a null pointer value, which we
+    // consider to be 'non-zero' here.
+    return !BaseOffs && !Scale && !(BaseGV && BaseReg);
   }
 
   Value *GetFieldAsValue(FieldName Field, Type *IntPtrTy) {
     switch (Field) {
     default:
       return nullptr;
     case BaseRegField:
       return BaseReg;

llvm/trunk/test/Transforms/CodeGenPrepare/ARM/sink-addrmode.ll

 ; CHECK: select i1 %cmp, i32* @gv1, i32* @gv2
 define void @test_select_trivial_gv(i32 %value) {
 entey:
   %cmp = icmp sgt i32 %value, 0
   %select = select i1 %cmp, i32* @gv1, i32* @gv2
   store i32 %value, i32* %select, align 4
   ret void
 }
+
+; Same for a select between a value and global variable
+; CHECK-LABEL: @test_select_trivial_ptr_gv
+; CHECK: select i1 %cmp, i32* %ptr, i32* @gv2
+define void @test_select_trivial_ptr_gv(i32* %ptr, i32 %value) {
+entry:
+  %cmp = icmp sgt i32 %value, 0
+  %select = select i1 %cmp, i32* %ptr, i32* @gv2
+  store i32 %value, i32* %select, align 4
+  ret void
+}
+
+; Same for a select between a global variable and null, though the test needs to
+; be a little more complicated to avoid dereferencing a potential null pointer
+; CHECK-LABEL: @test_select_trivial_gv_null
+; CHECK: select i1 %cmp.i, i32* @gv1, i32* null
+define void @test_select_trivial_gv_null(){
+entry:
+  %gv1_val = load i32, i32* @gv1, align 4
+  %cmp.i = icmp eq i32 %gv1_val, 0
+  %spec.select.i = select i1 %cmp.i, i32* @gv1, i32* null
+  br i1 %cmp.i, label %if.then, label %if.end
+
+if.then:
+  %val = load i32, i32* %spec.select.i, align 4
+  %inc = add nsw i32 %val, 1
+  store i32 %inc, i32* %spec.select.i, align 4
+  br label %if.end
+
+if.end:
+  ret void
+}
+
+; Same for a select between a value and null
+; CHECK-LABEL: @test_select_trivial_ptr_null
+; CHECK: select i1 %cmp.i, i32* %ptr, i32* null
+define void @test_select_trivial_ptr_null(i32* %ptr){
+entry:
+  %gv1_val = load i32, i32* %ptr, align 4
+  %cmp.i = icmp eq i32 %gv1_val, 0
+  %spec.select.i = select i1 %cmp.i, i32* %ptr, i32* null
+  br i1 %cmp.i, label %if.then, label %if.end
+
+if.then:
+  %val = load i32, i32* %spec.select.i, align 4
+  %inc = add nsw i32 %val, 1
+  store i32 %inc, i32* %spec.select.i, align 4
+  br label %if.end
+
+if.end:
+  ret void
+}