[LoopDataPrefetch/AArch64] Don't add prefetch intrinsic, when the loop already has InlineAsm prefetch.

[flyingforyou]

Summary

We need to check InliineAsm prefetch, not only Intrinsic::prefetch.

[hfinkel]

lib/Transforms/Scalar/LoopDataPrefetch.cpp

199

I think this would need to be some kind of TTI callback; we don't want to embed this kind of logic here. Also, I assume you'd need to check all of the lines, not just the first one.

[flyingforyou]

lib/Transforms/Scalar/LoopDataPrefetch.cpp

199

I think this would need to be some kind of TTI callback; we don't want to embed this kind of logic here.

Sure, Could you recommend callback function name please?

Also, I assume you'd need to check all of the lines, not just the first one.

Yes. I will change the StringRef's function to find from startswith_lower.

[flyingforyou]

Addressed Hal's comments.

Thanks.

[flyingforyou]

Is Diff 64111 too much modification?

I think we just check below string. Because it depends on ISA, only.
AArch64 : prfm
AArch32 : pld
PowerPC: dcbt
...

How about making helper function for checking these string?

or Just checking string in TargetTransformInfoImpl.h likes isLoweredToCall?

bool isLoweredToCall(const Function *F) {
  // FIXME: These should almost certainly not be handled here, and instead
  // handled with the help of TLI or the target itself. This was largely
  // ported from existing analysis heuristics here so that such refactorings
  // can take place in the future.

  if (F->isIntrinsic())
    return false;

  if (F->hasLocalLinkage() || !F->hasName())
    return true;

  StringRef Name = F->getName();

  // These will all likely lower to a single selection DAG node.
  if (Name == "copysign" || Name == "copysignf" || Name == "copysignl" ||
      Name == "fabs" || Name == "fabsf" || Name == "fabsl" || Name == "sin" ||
      Name == "fmin" || Name == "fminf" || Name == "fminl" ||
      Name == "fmax" || Name == "fmaxf" || Name == "fmaxl" ||
      Name == "sinf" || Name == "sinl" || Name == "cos" || Name == "cosf" ||
      Name == "cosl" || Name == "sqrt" || Name == "sqrtf" || Name == "sqrtl")
    return false;

  // These are all likely to be optimized into something smaller.
  if (Name == "pow" || Name == "powf" || Name == "powl" || Name == "exp2" ||
      Name == "exp2l" || Name == "exp2f" || Name == "floor" ||
      Name == "floorf" || Name == "ceil" || Name == "round" ||
      Name == "ffs" || Name == "ffsl" || Name == "abs" || Name == "labs" ||
      Name == "llabs")
    return false;

  return true;
}

Thanks,
Junmo Park.

[flyingforyou]

ping?

[flyingforyou]

ping2.

[t.p.northover]

This is a pretty nasty hack. The only other place I know of where we inspect inline asm (also poorly) is in BranchRelaxation when we need to know how big it is. This is justified only by the fact that it's a major correctness issue if we don't.

We have no idea if the prefetch is for the address we're considering, or even if it actually exists! (e.g. asm volatile("; We don't want a prfm here\n\t" ...)).

IMO, users should be encouraged to use the ACLE intrinsics instead. They produce completely sensible @llvm.prefetch instructions LLVM can reason about properly.

[flyingforyou]

Thanks for the comment Tim.

We have no idea if the prefetch is for the address we're considering.

This is original algorithm's behavior. When you see the comment over the checking routine, there is mentioned likes below.

// If the loop already has prefetches, then assume that the user knows
// what they are doing and don't add any more.

even if it actually exists! (e.g. asm volatile("; We don't want a prfm here\n\t" ...)).

This can be fixed when we make check routine more robust.

IMO, users should be encouraged to use the ACLE intrinsics instead.

I am not sure that all users will use ACLE intrinsics instead of inline asm. I think using inline asm or intrinsic is still user's choice. Because inline asm has much longer history than intrinsic. And there are so many codes which use inline asm.
If I miss something, please let me know.

[flyingforyou]

Or how about give up inserting prefetch intrinsic when the loop has inline asm?
I don't think users are stupid that they don't know prefetch is necessary or not when they use inline asm.

[t.p.northover]

I am not sure that all users will use ACLE intrinsics instead of inline asm.

They should always use intrinsics when standardized ones are available. Usually even when non-standard ones are. They're strictly more portable than asm, and one of the main reasons we advocate them is precisely because they give the compiler a better idea of what's happening (otherwise we could quite easily have implemented arm_neon.h using inline asm, like GCC).

I think using inline asm or intrinsic is still user's choice. Because inline asm has much longer history than intrinsic. And there are so many codes which use inline asm.

It's certainly their choice, but one penalty of using asm has always been that the compiler doesn't know what's going on. I'm pretty strongly opposed to changing that.

Or how about give up inserting prefetch intrinsic when the loop has inline asm?

I could just about live with that.

[flyingforyou]

OK Tim.

Hal, If you also agree with this, I will abandon this patch.

[hfinkel]

In D22399#504287, @flyingforyou wrote:

OK Tim.

Hal, If you also agree with this, I will abandon this patch.

I agree.

[flyingforyou]

Thanks. Hal, Tim.

lib/Transforms/Scalar/LoopDataPrefetch.cpp

 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/IntrinsicInst.h"
+#include "llvm/IR/InlineAsm.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"

     // what they are doing and don't add any more.
     for (BasicBlock::iterator J = (*I)->begin(), JE = (*I)->end();
          J != JE; ++J)
-      if (CallInst *CI = dyn_cast<CallInst>(J))
-        if (Function *F = CI->getCalledFunction())
+      if (CallInst *CI = dyn_cast<CallInst>(J)) {
+        if (const InlineAsm *IA = dyn_cast<InlineAsm>(CI->getCalledValue())) {
+          // TODO: Need to check other architectures.
+          if (StringRef(IA->getAsmString()).startswith_lower("prfm"))
+            return MadeChange;
+        } else if (Function *F = CI->getCalledFunction()) {
           if (F->getIntrinsicID() == Intrinsic::prefetch)
             return MadeChange;
+        }
+      }
 
     Metrics.analyzeBasicBlock(*I, *TTI, EphValues);
   }

test/Transforms/LoopDataPrefetch/AArch64/check-asm.ll

+; RUN: opt -mcpu=cyclone -mtriple=arm64-apple-ios -loop-data-prefetch -max-prefetch-iters-ahead=1000 -min-prefetch-stride=1 -S < %s | FileCheck %s
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-n32:64-S128"
+
+define void @no_prefetch(double* nocapture %a, double* nocapture readonly %b) {
+entry:
+  br label %for.body
+
+; ALL: for.body:
+for.body:                                         ; preds = %for.body, %entry
+  %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.body ]
+  %arrayidx = getelementptr inbounds double, double* %b, i64 %indvars.iv
+; CHECK-NOT: call void @llvm.prefetch
+  tail call void asm sideeffect "prfm PLDL1KEEP, [$0, $1]", "r,n"(double* %arrayidx, i32 0)
+  %0 = load double, double* %arrayidx, align 8
+  %add = fadd double %0, 1.000000e+00
+  %arrayidx2 = getelementptr inbounds double, double* %a, i64 %indvars.iv
+  store double %add, double* %arrayidx2, align 8
+  %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
+  %exitcond = icmp eq i64 %indvars.iv.next, 1600
+  br i1 %exitcond, label %for.end, label %for.body
+
+; ALL: for.end:
+for.end:                                          ; preds = %for.body
+  ret void
+}
+