[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<Instruction*> &SafeToPromote);
 };
 
 class ARMCodeGenPrepare : public FunctionPass {
   const ARMSubtarget *ST = nullptr;
   IRPromoter *Promoter = nullptr;
   std::set<Value*> AllVisited;
   SmallPtrSet<Instruction*, 8> SafeToPromote;
+  SmallPtrSet<Function*, 4> SelUsers;
+  bool SafeToWriteGE = false;
 
   bool isSafeOverflow(Instruction *I);
   bool isSupportedValue(Value *V);
   bool isLegalToPromote(Value *V);
   bool TryToPromote(Value *V);
 
 public:
   static char ID;
void IRPromoter::Mutate(Type *OrigTy,
 
   // Insert trunc instructions for use by calls, stores etc...
   TruncateSinks(Sources, Sinks);
 
   // Finally, remove unecessary zexts and truncs, delete old instructions and
   // clear the data structures.
   Cleanup(Sinks);
 
-  LLVM_DEBUG(dbgs() << "ARM CGP: Mutation complete:\n");
-  LLVM_DEBUG(dbgs();
-             for (auto *V : Sources)
-               V->dump();
-             for (auto *I : NewInsts)
-               I->dump();
-             for (auto *V : Visited) {
-               if (!Sources.count(V))
-                 V->dump();
-              });
+  LLVM_DEBUG(dbgs() << "ARM CGP: Mutation complete.\n");
 }
 
 /// 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))
bool ARMCodeGenPrepare::isLegalToPromote(Value *V) {
   if (isPromotedResultSafe(V) || isSafeOverflow(I)) {
     SafeToPromote.insert(I);
     return true;
   }
 
   if (I->getOpcode() != Instruction::Add && I->getOpcode() != Instruction::Sub)
     return false;
 
+  // Predicated on whether the sel intrinsic has been used within the function.
+  // 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, SafeToPromote);
   return true;
 }
 
 bool ARMCodeGenPrepare::doInitialization(Module &M) {
   Promoter = new IRPromoter(&M);
+  auto Sel = Intrinsic::getDeclaration(&M, Intrinsic::arm_sel);
+  for (auto &U : Sel->uses())
+    SelUsers.insert(cast<Instruction>(U.getUser())->getParent()->getParent());
   return false;
 }
 
 bool ARMCodeGenPrepare::runOnFunction(Function &F) {
   if (skipFunction(F) || DisableCGP)
     return false;
 
   auto *TPC = &getAnalysis<TargetPassConfig>();
   if (!TPC)
     return false;
 
   const TargetMachine &TM = TPC->getTM<TargetMachine>();
   ST = &TM.getSubtarget<ARMSubtarget>(F);
   bool MadeChange = false;
   LLVM_DEBUG(dbgs() << "ARM CGP: Running on " << F.getName() << "\n");
 
+  SafeToWriteGE = SelUsers.count(&F) == 0;
+  LLVM_DEBUG(if (SafeToWriteGE)
+               dbgs() << "ARM CGP: Can use GE writing intrinsics.\n");
+
   // Search up from icmps to try to promote their operands.
   for (BasicBlock &BB : F) {
     auto &Insts = BB.getInstList();
     for (auto &I : Insts) {
       if (AllVisited.count(&I))
         continue;
 
       if (isa<ICmpInst>(I)) {

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

+; RUN: llc -mtriple=thumbv7em %s -arm-disable-cgp=false -o - | FileCheck %s --check-prefix=NODSP
+; RUN: llc -mtriple=thumbv8m.main -mcpu=cortex-m33 -arm-disable-cgp=false -arm-enable-scalar-dsp=true %s -o - | FileCheck %s --check-prefix=DSP
+
+; CHECK-LABEL:  dsp_enable:
+; NODSP-NOT:    usub
+; NODSP:        uxtb
+; DSP:          usub8
+define void @dsp_enable(i8* %in, i8* %out, i8 zeroext %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
+  %sub = sub i8 %ld0, %ld1
+  %xor = xor i8 %ld0, -1
+  %cmp = icmp ult i8 %compare, %sub
+  %select = select i1 %cmp, i8 %sub, i8 %xor
+  store i8 %select, i8* %out, align 1
+  ret void
+}
+
+; CHECK-LABEL:  dsp_disable:
+; NODSP-NOT:    usub
+; DSP-NOT:      usub
+; CHECK:        uxtb
+define i32 @dsp_disable(i8* %in, i8* %out, i8 zeroext %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
+  %sub = sub i8 %ld0, %ld1
+  %xor = xor i8 %ld0, -1
+  %cmp = icmp ult i8 %compare, %sub
+  %select = select i1 %cmp, i8 %sub, i8 %xor
+  store i8 %select, i8* %out, align 1
+  %a = zext i8 %ld0 to i32
+  %b = zext i8 %ld1 to i32
+  %sel = call i32 @llvm.arm.sel(i32 %a, i32 %b)
+  ret i32 %sel
+}
+
+declare i32 @llvm.arm.sel(i32, i32)