[ARM] Allow pointer values in ARMCodeGenPrepare

[samparker]

Summary

Skip pointer values so they don't unnecessarily prevent the transformation.

[samparker]

Rebased and fixed the handling of undef values. I've also moved the tests around so we have a standalone file for the call tests. Also added a small piece of control to decide whether we bother to promote or not: if we find nothing but sources, sinks and the icmp, then we don't bother doing anything.

[SjoerdMeijer]

Nit picking the subject/description:

Ignore pointer values in ARMCodeGenPrepare

and

Skip pointer values so they don't unnecessarily prevent the transformation.

But pointer types are now supported types. Thus you're actually *allowing* them, not ignoring/skipping. Not only is 'allowing' a more positive twist, I also think it is clearer :)

lib/Target/ARM/ARMCodeGenPrepare.cpp

634

Do you need to test this again? It looks like this is already done in a few place, e.g. line 588.

test/CodeGen/ARM/arm-cgp-calls.ll

93 ↗

(On Diff #160575)

Do you want to test something here?

[samparker]

lib/Target/ARM/ARMCodeGenPrepare.cpp

634

Good point, Leaves will contain all the non-instruction values. I should be able to remove all of these checks.

test/CodeGen/ARM/arm-cgp-calls.ll

93 ↗

(On Diff #160575)

That would be nice wouldn't it? :)

[samparker]

Removed the unnecessary isa<Instruction> checks and updated the test to actually test.

[SjoerdMeijer]

Looks okay to me

lib/Target/ARM/ARMCodeGenPrepare.cpp

   auto *I = dyn_cast<Instruction>(V);
   if (!I)
     return false;
 
   if (!isa<IntegerType>(V->getType()))
     return false;
 
   if (isa<StoreInst>(I) || isa<TerminatorInst>(I) || isa<TruncInst>(I) ||
-      isa<ICmpInst>(I))
+      isa<ICmpInst>(I) || isa<GetElementPtrInst>(V))
     return false;
 
   if (auto *ZExt = dyn_cast<ZExtInst>(I))
     return !ZExt->getDestTy()->isIntegerTy(32);
 
   return true;
 }
 

 
   // Iterate through, and add to, a tree of operands and users in the use-def.
   while (!WorkList.empty()) {
     Value *V = WorkList.back();
     WorkList.pop_back();
     if (CurrentVisited.count(V))
       continue;
 
-    if (!isa<Instruction>(V) && !isSource(V))
+    // Ignore pointers and non-instructions, other than arguments.
+    if (isa<PointerType>(V->getType()) ||
+        (!isa<Instruction>(V) && !isSource(V)))
       continue;
 
     // If we've already visited this value from somewhere, bail now because
     // the tree has already been explored.
     // TODO: This could limit the transform, ie if we try to promote something
     // from an i8 and fail first, before trying an i16.
     if (AllVisited.count(V)) {
       LLVM_DEBUG(dbgs() << "ARM CGP: Already visited this: " << *V << "\n");

       // Visit operands of any instruction visited.
       for (auto &U : I->operands()) {
         if (!AddLegalInst(U))
           return false;
       }
     }
 
     // Don't visit users of a node which isn't going to be mutated unless its a
     // source.
     if (isSource(V) || shouldPromote(V)) {
       for (Use &U : V->uses()) {
         if (!AddLegalInst(U.getUser()))
           return false;
       }
     }
   }
 

test/CodeGen/ARM/arm-cgp-icmps.ll

 
 for.inc:
   %add = add nuw i8 %cond.in.i.off0, 1
   br label %body
 
 exit:
   ret void
 }
+

test/CodeGen/ARM/arm-cgp-phis-calls-ret.ll

if.end:
   %inc2 = phi i8 [ %inc, %if.then], [ %inc1, %if.else ]
   %cmp1 = icmp eq i8 %inc2, 255
   br i1 %cmp1, label %exit, label %loop
 
 exit:
   ret void
 }
 
+; CHECK-COMMON-LABEL: phi_pointers
+; CHECK-COMMON-NOT: uxt
+define void @phi_pointers(i16* %a, i16* %b, i8 zeroext %M, i8 zeroext %N) {
+entry:
+  %add = add nuw i8 %M, 1
+  %and = and i8 %add, 1
+  %cmp = icmp ugt i8 %add, %N
+  %base = select i1 %cmp, i16* %a, i16* %b
+  %other = select i1 %cmp, i16* %b, i16* %b
+  br label %loop
+
+loop:
+  %ptr = phi i16* [ %base, %entry ], [ %gep, %loop ]
+  %idx = phi i8 [ %and, %entry ], [ %inc, %loop ]
+  %load = load i16, i16* %ptr, align 2
+  %inc = add nuw nsw i8 %idx, 1
+  %gep = getelementptr inbounds i16, i16* %ptr, i8 %inc
+  %cond = icmp eq i16* %gep, %other
+  br i1 %cond, label %exit, label %loop
+
+exit:
+  ret void
+}
+
+; CHECK-COMMON-LABEL: phi_pointers_null
+; CHECK-COMMON-NOT: uxt
+define void @phi_pointers_null(i16* %a, i16* %b, i8 zeroext %M, i8 zeroext %N) {
+entry:
+  %add = add nuw i8 %M, 1
+  %and = and i8 %add, 1
+  %cmp = icmp ugt i8 %add, %N
+  %base = select i1 %cmp, i16* %a, i16* %b
+  %other = select i1 %cmp, i16* %b, i16* %b
+  %cmp.1 = icmp eq i16* %base, %other
+  br i1 %cmp.1, label %fail, label %loop
+
+fail:
+  br label %loop
+
+loop:
+  %ptr = phi i16* [ %base, %entry ], [ null, %fail ], [ %gep, %if.then ]
+  %idx = phi i8 [ %and, %entry ], [ 0, %fail ], [ %inc, %if.then ]
+  %undef = icmp eq i16* %ptr, undef
+  br i1 %undef, label %exit, label %if.then
+
+if.then:
+  %load = load i16, i16* %ptr, align 2
+  %inc = add nuw nsw i8 %idx, 1
+  %gep = getelementptr inbounds i16, i16* %ptr, i8 %inc
+  %cond = icmp eq i16* %gep, %other
+  br i1 %cond, label %exit, label %loop
+
+exit:
+  ret void
+}
+
+declare i8 @do_something_with_ptr(i8, i16*)
+
+; CHECK-COMMON-LABEL: call_pointer
+; CHECK-COMMON-NOT: uxt
+define i8 @call_pointer(i8 zeroext %x, i8 zeroext %y, i16* %a, i16* %b) {
+  %or = or i8 %x, %y
+  %shr = lshr i8 %or, 1
+  %add = add nuw i8 %shr, 2
+  %cmp = icmp ne i8 %add, 0
+  %ptr = select i1 %cmp, i16* %a, i16* %b
+  %call = tail call zeroext i8 @do_something_with_ptr(i8 %shr, i16* %ptr)
+  ret i8 %call
+}
+
 ; Just check that phis also work with i16s.
 ; CHECK-COMMON-LABEL: phi_i16:
 ; CHECK-COMMON-NOT:   uxt
 define void @phi_i16() {
 entry:
   br label %loop
 
 loop: