[ARM] Cleanup ARM CGP isSupportedValue

[samparker]

Summary

isSupportedValue explicitly checked and accepted many types of value, primarily for debugging reasons. Remove most of these checks and do a bit of refactoring now that the pass is more stable. This also enables ZExts to be sources, but this has very little practical benefit at the moment extend instructions will still be introduced.

[SjoerdMeijer]

Looks like a straightforward addition/cleanup to me.

[vsapsai]

Looks like this changed causes errors in backend in stage 2 on Apple platforms

Argument value type does not match pointer operand type!
  %0 = atomicrmw volatile xchg i8* %_Value1, i32 1 monotonic, !dbg !26
 i8in function atomic_flag_test_and_set
fatal error: error in backend: Broken function found, compilation aborted!

Will provide more details shortly.

[vsapsai]

I've reduced the input causing error to

typedef _Atomic(_Bool)              atomic_bool;

typedef struct atomic_flag { atomic_bool _Value; } atomic_flag;

_Bool atomic_flag_test_and_set(volatile atomic_flag *object) {
  return __c11_atomic_exchange(&(object)->_Value, 1, __ATOMIC_SEQ_CST);
}

And clang encounters the fatal error when invoked as

"bin/clang-8" "-cc1" "-triple" "thumbv6m-apple-unknown-macho" "-emit-obj" "-disable-free" "-static-define" "-mrelocation-model" "static" "-mthread-model" "posix" "-masm-verbose" "-mconstructor-aliases" "-ffreestanding" "-target-cpu" "cortex-m0" "-target-feature" "+soft-float" "-target-feature" "+soft-float-abi" "-target-feature" "-fp-only-sp" "-target-feature" "-d16" "-target-feature" "-vfp2" "-target-feature" "-vfp3" "-target-feature" "-fp16" "-target-feature" "-vfp4" "-target-feature" "-fp-armv8" "-target-feature" "-neon" "-target-feature" "-crypto" "-target-feature" "+strict-align" "-target-abi" "aapcs" "-msoft-float" "-mfloat-abi" "soft" "-fallow-half-arguments-and-returns" "-sys-header-deps" "-D" "NDEBUG" "-Oz" "-Wall" "-ferror-limit" "19" "-fmessage-length" "150" "-fno-signed-char" "-fobjc-runtime=gnustep" "-fdiagnostics-show-option" "-fcolor-diagnostics" "-vectorize-slp" "-x" "c" repro.c

I will revert the commit.

[samparker]

Thanks for the reproducer and for the revert.

lib/Target/ARM/ARMCodeGenPrepare.cpp

 };
 
 }
 
 static bool generateSignBits(Value *V) {
   if (!isa<Instruction>(V))
     return false;
 
+  if (isa<SExtInst>(V))
+    return true;
+
   unsigned Opc = cast<Instruction>(V)->getOpcode();
   return Opc == Instruction::AShr || Opc == Instruction::SDiv ||
          Opc == Instruction::SRem;
 }
 
 /// Some instructions can use 8- and 16-bit operands, and we don't need to
 /// promote anything larger. We disallow booleans to make life easier when
 /// dealing with icmps but allow any other integer that is <= 16 bits. Void

 /// of the tree to i32. We guarantee that these won't populate the upper bits
 /// of the register. ZExt on the loads will be free, and the same for call
 /// return values because we only accept ones that guarantee a zeroext ret val.
 /// Many arguments will have the zeroext attribute too, so those would be free
 /// too.
 static bool isSource(Value *V) {
   if (!isa<IntegerType>(V->getType()))
     return false;
-  // TODO Allow zext to be sources.
-  if (isa<Argument>(V))
-    return true;
-  else if (isa<LoadInst>(V))
+  else if (isa<Argument>(V) || isa<LoadInst>(V) || isa<CallInst>(V))
     return true;
-  else if (isa<BitCastInst>(V))
-    return true;
-  else if (auto *Call = dyn_cast<CallInst>(V))
-    return Call->hasRetAttr(Attribute::AttrKind::ZExt);
-  else if (auto *Trunc = dyn_cast<TruncInst>(V))
-    return isSupportedType(Trunc);
+  else if (isa<CastInst>(V))
+    return isSupportedType(V);
   return false;
 }
 
 /// Return true if V will require any promoted values to be truncated for the
 /// the IR to remain valid. We can't mutate the value type of these
 /// instructions.
 static bool isSink(Value *V) {
   // TODO The truncate also isn't actually necessary because we would already
LLVM_DEBUG(dbgs();
               });
 }
 
 /// We accept most instructions, as well as Arguments and ConstantInsts. We
 /// Disallow casts other than zext and truncs and only allow calls if their
 /// return value is zeroext. We don't allow opcodes that can introduce sign
 /// bits.
 bool ARMCodeGenPrepare::isSupportedValue(Value *V) {
-  if (isa<ICmpInst>(V))
-    return true;
+  if (generateSignBits(V))
+    return false;
 
-  // Memory instructions
-  if (isa<StoreInst>(V) || isa<GetElementPtrInst>(V))
-    return true;
+  // Disallow for simplicity.
+  if (isa<ConstantExpr>(V))
+    return false;
 
-  // Branches and targets.
-  if( isa<BranchInst>(V) || isa<SwitchInst>(V) || isa<BasicBlock>(V))
+  // Special case because they generate an i1, which we don't generally
+  // support.
+  if (isa<ICmpInst>(V))
     return true;
 
-  // Non-instruction values that we can handle.
-  if ((isa<Constant>(V) && !isa<ConstantExpr>(V)) || isa<Argument>(V))
-    return isSupportedType(V);
-
-  if (isa<PHINode>(V) || isa<SelectInst>(V) || isa<ReturnInst>(V) ||
-      isa<LoadInst>(V))
-    return isSupportedType(V);
-
-  // Truncs can be either sources or sinks.
-  if (auto *Trunc = dyn_cast<TruncInst>(V))
-    return isSupportedType(Trunc) || isSupportedType(Trunc->getOperand(0));
-
-  if (isa<CastInst>(V) && !isa<SExtInst>(V))
-    return isSupportedType(cast<CastInst>(V)->getOperand(0));
+  // Both ZExts and Truncs can be either sources and sinks. BitCasts are also
+  // sources and SExts are disallowed through their sign bit generation.
+  if (auto *Cast = dyn_cast<CastInst>(V))
+    return isSupportedType(Cast) || isSupportedType(Cast->getOperand(0));
 
   // Special cases for calls as we need to check for zeroext
   // TODO We should accept calls even if they don't have zeroext, as they can
   // still be sinks.
   if (auto *Call = dyn_cast<CallInst>(V))
     return isSupportedType(Call) &&
            Call->hasRetAttr(Attribute::AttrKind::ZExt);
 
-  if (!isa<BinaryOperator>(V))
-    return false;
-
-  if (!isSupportedType(V))
-    return false;
-
-  if (generateSignBits(V)) {
-    LLVM_DEBUG(dbgs() << "ARM CGP: No, instruction can generate sign bits.\n");
-    return false;
-  }
-  return true;
+  return isSupportedType(V);
 }
 
 /// Check that the type of V would be promoted and that the original type is
 /// smaller than the targeted promoted type. Check that we're not trying to
 /// promote something larger than our base 'TypeSize' type.
 bool ARMCodeGenPrepare::isLegalToPromote(Value *V) {
   if (isPromotedResultSafe(V))
     return true;

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

if.end:
   %gep1 = getelementptr inbounds i8, i8* %ptr, i32 %inc
   %3 = load i8, i8* %gep1, align 1
   br label %body
 
 exit:
   ret i8 %2
 }
 
+; TODO: We should be able to remove both extends from this example, by looking
+; back through the zexts and accepting a type size <= than the icmp.
+; CHECK-COMMON-LABEL: icmp_i16_zext
+; CHECK-COMMON: uxt
+; CHECK-COMMON: uxt
+define i8 @icmp_i16_zext(i8* %ptr) {
+entry:
+  %gep = getelementptr inbounds i8, i8* %ptr, i32 0
+  %0 = load i8, i8* %gep, align 1
+  %1 = sub nuw nsw i8 %0, 1
+  %conv44 = zext i8 %0 to i16
+  br label %preheader
+
+preheader:
+  br label %body
+
+body:
+  %2 = phi i8 [ %1, %preheader ], [ %3, %if.end ]
+  %si.0274 = phi i16 [ %conv44, %preheader ], [ %inc, %if.end ]
+  %conv51266 = zext i8 %2 to i16
+  %cmp52267 = icmp eq i16 %si.0274, %conv51266
+  br i1 %cmp52267, label %if.end, label %exit
+
+if.end:
+  %inc = add nuw i16 %si.0274, 1
+  %conv = zext i16 %inc to i32
+  %gep1 = getelementptr inbounds i8, i8* %ptr, i32 %conv
+  %3 = load i8, i8* %gep1, align 1
+  br label %body
+
+exit:
+  ret i8 %2
+}
+
 ; Won't don't handle sext
 ; CHECK-COMMON-LABEL: icmp_sext_zext_store_i8_i16
 ; CHECK-COMMON: ldrb
 ; CHECK-COMMON: ldrsh
 define i32 @icmp_sext_zext_store_i8_i16() {
 entry:
   %0 = load i8, i8* getelementptr inbounds ([16 x i8], [16 x i8]* @d_uch, i32 0, i32 2), align 1
   %conv = zext i8 %0 to i16