[ARM] Check for sel intrinsic use in ARM CGP

[samparker]

Summary

During initialisation, check whether the llvm.arm.sel intrinsic is used in the current module. If it has, we conservatively choose that it would be unsafe to generate the DSP add/sub instructions that will write to the GE flags - the same flags which sel will read.

[efriedma]

This isn't really correct, in any useful sense... we can't assume the current translation unit reflects the entire program.

I mean, if you really think this optimization is sufficiently useful, we can add a flag for it. Presumably user code could enable it if they want to, but it would have to be disabled for libraries shipped with the compiler (like compiler-rt).

[samparker]

Hi Eli,

I don't intend to remove the -arm-enable-scalar-dsp flag or change the default from false, meaning that the user will have to enable it and even then the pass can decide against it. From your comments in D48832 and John's in D49239, this seems like the most conservative approach to me. By the ACLE standard I don't have to worry about the whole program and I could just look at the current function. I will want to use the same approach in the parallel dsp pass but I want to be as safe as possible.

[john.brawn]

Because the ABI says that the GE bits are undefined on entry / return from public interfaces (i.e. non-static functions) I think that it is possible to just look at the current translation unit and make a decision based on that. I'm not sure if just looking at whether the sel intrinsic is used will work though - the GE bits could be read by __arm_rsr("APSR"), or by inline assembly, and maybe there are other situations that I haven't thought of.

[samparker]

Yes, it's certainly complicating that the ABI and ACLE don't talk about inline assembly or other intrinsics and this is why I still want to have the option guarded by a command line option. I will update the patch to look at the function only.

[samparker]

On initialisation, record the functions that contain uses of the sel intrinsic so that we now check on a function basis and not module.

lib/Target/ARM/ARMCodeGenPrepare.cpp

void Mutate(Type *OrigTy,
               SmallPtrSetImpl<Value*> &Sources,
               SmallPtrSetImpl<Instruction*> &Sinks);
 };
 
 class ARMCodeGenPrepare : public FunctionPass {
   const ARMSubtarget *ST = nullptr;
   IRPromoter *Promoter = nullptr;
   std::set<Value*> AllVisited;
+  bool SafeToWriteGE = false;
 
   bool isSupportedValue(Value *V);
   bool isLegalToPromote(Value *V);
   bool TryToPromote(Value *V);
 
 public:
   static char ID;
   static unsigned TypeSize;

 bool ARMCodeGenPrepare::isLegalToPromote(Value *V) {
   if (isPromotedResultSafe(V))
     return true;
 
   auto *I = dyn_cast<Instruction>(V);
   if (!I)
     return false;
 
+  // Predicated on whether the sel intrinsic has been used within the module.
+  // If it has, conservatively choose to not generate any instructions that
+  // would overwrite the GE flags.
+  if (!SafeToWriteGE)
+    return false;
+
   // If promotion is not safe, can we use a DSP instruction to natively
   // handle the narrow type?
   if (!ST->hasDSP() || !EnableDSP || !isSupportedType(I))
     return false;
 
   if (ST->isThumb() && !ST->hasThumb2())
     return false;
 
if (ToPromote < 2)
     return false;
 
   Promoter->Mutate(OrigTy, CurrentVisited, Sources, Sinks);
   return true;
 }
 
 bool ARMCodeGenPrepare::doInitialization(Module &M) {
   Promoter = new IRPromoter(&M);
+  SafeToWriteGE =
+    Intrinsic::getDeclaration(&M, Intrinsic::arm_sel)->hasNUses(0);
   return false;
 }
 
 bool ARMCodeGenPrepare::runOnFunction(Function &F) {
   if (skipFunction(F) || DisableCGP)
     return false;
 
   auto *TPC = &getAnalysis<TargetPassConfig>();

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

+; RUN: llc -mtriple=thumbv7em %s -o - | FileCheck %s
+; RUN: llc -mtriple=thumbv8m.main -mcpu=cortex-m33 -arm-enable-scalar-dsp=true %s -o - | FileCheck %s
+; RUN: llc -mtriple=thumbv8m.main -mcpu=cortex-m33 -arm-enable-scalar-dsp=true -arm-enable-scalar-dsp-imms=true %s -o - | FileCheck %s
+
+define i32 @use_sel(i32 %a, i32 %b) {
+  %1 = call i32 @llvm.arm.sel(i32 %a, i32 %b)
+  ret i32 %1
+}
+
+; Test that because the sel intrinsic is used above, we don't generate a dsp
+; sub.
+; CHECK-LABEL: dsp_disable:
+; CHECK-NOT: usub
+; CHECK-NOT: ssub
+define void @dsp_disable(i8* %in, i8* %out, i8 %compare) {
+  %first = getelementptr inbounds i8, i8* %in, i32 0
+  %second = getelementptr inbounds i8, i8* %in, i32 1
+  %ld0 = load i8, i8* %first
+  %ld1 = load i8, i8* %second
+  %xor = xor i8 %ld0, -1
+  %cmp = icmp ult i8 %compare, %ld1
+  %select = select i1 %cmp, i8 %compare, i8 %xor
+  %sub = sub i8 %ld0, %select
+  store i8 %sub, i8* %out, align 1
+  ret void
+}
+
+declare i32 @llvm.arm.sel(i32, i32)