[IndirectFunctions] Skip propagating attributes to address taken functions

[madhur13490]

Summary

In case of indirect calls or address taken functions,
skip propagating any attributes to them. We just
propagate features to such functions.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp

240–245

Propagating the subtarget features is broken. The cases that need this both should be set up front, or shouldn't be subtarget features

llvm/test/CodeGen/AMDGPU/propagate-attributes-direct-indirect.ll

41

CHECK-LABEL, and don't need the arguments/attribute group

44–64

This can be simplified. You shouldn't need allocas or addrspacecasts

[rampitec]

With this patch you would set features on an address-taken function and ignore whole call stack below it. I.e. its own callees will not be processed. I think you need to continue traversal, just skip actual setting of attributes on such a function. Setting these attributes on a functions it may call in turn shall be fine.

llvm/test/CodeGen/AMDGPU/propagate-attributes-direct-indirect.ll

58

There should be no numbered variables in tests.

[madhur13490]

rebase + address comments + fix test

[madhur13490]

In D94585#2496288, @rampitec wrote:

With this patch you would set features on an address-taken function and ignore whole call stack below it. I.e. its own callees will not be processed. I think you need to continue traversal, just skip actual setting of attributes on such a function. Setting these attributes on a functions it may call in turn shall be fine.

Decided to skip both features and attributes to address taken functions as discussed offline.

[rampitec]

llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp

242

I think you still need to add it to NewRoots to propagate from F down the stack.

[arsenm]

llvm/test/CodeGen/AMDGPU/propagate-attributes-common-callees.ll

77–78 ↗

(On Diff #316862)

Don't need all this, most of this is noise

[madhur13490]

Address Matt's comments

[madhur13490]

Bump!

llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp

242

I don't think that is needed. The address taken functions will neither have features not attributes to no need to propagate them to their callees. If a function is called from both address taken function and a non-address taken function then the traversal would address that.

[rampitec]

llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp

242

I'd say never say never. You had a proper and simple code to add it the roots in this if() block.

[madhur13490]

Address Stas's comment

[madhur13490]

rebase

[rampitec]

llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp

242

NewRoots is a set, there is no need to check for the NewRoots.count(F), just insert.

244

It is not really changed yet.

271

Do you still need this part? It was needed when you was doing partial update of the properties, which you do not do now.

llvm/test/CodeGen/AMDGPU/propagate-attributes-direct-indirect.ll

78

This can be cleaned a lot.

[madhur13490]

llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp

244

Ok

271

Yes, it is still needed. Traversal may go into infinite loop, for example, the common-callees test depicts one scenario where we need this condition for convergence.

llvm/test/CodeGen/AMDGPU/propagate-attributes-direct-indirect.ll

78

How? These are features being propagated from direct callees.

[rampitec]

llvm/test/CodeGen/AMDGPU/propagate-attributes-direct-indirect.ll

78

These are not features we check for or propagate. You can reduce the list to only the features we really need to propagate.

[madhur13490]

Remove changed

[madhur13490]

llvm/test/CodeGen/AMDGPU/propagate-attributes-direct-indirect.ll

78

Not sure if I understood. These are the features propagated by the "early" version to the function. If you mean just checking the attribute ID i.e. "#0" then I am not sure what value it adds.

[rampitec]

llvm/test/CodeGen/AMDGPU/propagate-attributes-direct-indirect.ll

78

You do not need all that long list: +16-bit-insts,+add-no-carry-insts,+aperture-regs,+ci-insts,+dl-insts,+dot1-insts,+dot2-insts,+dpp,+ds-src2-insts,+enable-ds128,+enable-prt-strict-null,+fast-denormal-f32,+fast-fmaf,+flat-address-space,+flat-for-global,+flat-global-insts,+flat-inst-offsets,+flat-scratch-insts,+fma-mix-insts,+fp64,+gcn3-encoding,+gfx7-gfx8-gfx9-insts,+gfx8-insts,+gfx9,+gfx9-insts,+half-rate-64-ops,+image-gather4-d16-bug,+int-clamp-insts,+inv-2pi-inline-imm,+ldsbankcount32,+load-store-opt,+localmemorysize65536,+mad-mac-f32-insts,+no-xnack-support,+promote-alloca,+r128-a16,+s-memrealtime,+s-memtime-inst,+scalar-atomics,+scalar-flat-scratch-insts,+scalar-stores,+sdwa,+sdwa-omod,+sdwa-scalar,+sdwa-sdst,+sram-ecc,+trap-handler,+unaligned-access-mode,+unaligned-buffer-access,+unaligned-ds-access,+vgpr-index-mode,+vop3p

[madhur13490]

Remove unnecessary things from feature list

[rampitec]

llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp

242

Just

if (!Roots.count(&F))
  NewRoots.insert(&F);

NewRoots is a set.

llvm/test/CodeGen/AMDGPU/propagate-attributes-common-callees.ll

11 ↗

(On Diff #317918)

95% of the code can be removed, it is not needed for the test. Here and in other places. You only need empty functions and calls.

llvm/test/CodeGen/AMDGPU/propagate-attributes-direct-indirect.ll

78

It still can be cleaned. +xnack is not needed. Most of the stuff in #1 is not needed too. Here and in other places.

[madhur13490]

simplify tests; remove align, add dummy body; remove input param

[rampitec]

LGTM, just remove "target datalayout" from all tests before you submit. I believe it is not needed.

[madhur13490]

Remove target layout and addrspace

[madhur13490]

update summary

[madhur13490]

rebase

[kzhuravl]

This change is causing infinite loop when compiling rocThrust and hipCUB. Can you take a look? Thanks.

[madhur13490]

In D94585#2520966, @kzhuravl wrote:

This change is causing infinite loop when compiling rocThrust and hipCUB. Can you take a look? Thanks.

FWIW, after testing rocThrust and hipCUB, it turned out that, it is not an infinite loop but a rise in compile-time which crossed timing threshold of the internal infra. The apps eventually compiled with 1-2% increment in the compile-time. I figured out the cause behind this. This patch makes two additional calls to Function::hasAddressTaken() and hasAddressTaken() is not optimal. Each time Function::hasAddressTaken() is called, it traverses over all uses of the function to deduce the required info. The calls made by this patch are itself in the loop which effectively made the suboptimality of Function::hasAddressTaken() conspicuous. In the new patch I am going to remove one call to Function::hasAddressTaken() which still preserves the intention of this patch. The new patch would basically be Diff3 of this patch.

We should think about optimizing Function::hasAddressTaken() by introducing a bool in Function class which would cache the result. In addition to this, Function::hasAddressTaken() can accept an additional bool parameter and based on its truthness, the function can return the cached value. The default value of parameter bool should be false to preserve today's behavior but client can set it to "true" if they want the cached value. Latter would be useful for this patch as the information is unlikely to change in a module (which is static naturally). This optimization would significantly improve the run-time of hasAddressTaken().

[kzhuravl]

In D94585#2524476, @madhur13490 wrote:

In D94585#2520966, @kzhuravl wrote:

This change is causing infinite loop when compiling rocThrust and hipCUB. Can you take a look? Thanks.

FWIW, after testing rocThrust and hipCUB, it turned out that, it is not an infinite loop but a rise in compile-time which crossed timing threshold of the internal infra. The apps eventually compiled with 1-2% increment in the compile-time. I figured out the cause behind this. This patch makes two additional calls to Function::hasAddressTaken() and hasAddressTaken() is not optimal. Each time Function::hasAddressTaken() is called, it traverses over all uses of the function to deduce the required info. The calls made by this patch are itself in the loop which effectively made the suboptimality of Function::hasAddressTaken() conspicuous. In the new patch I am going to remove one call to Function::hasAddressTaken() which still preserves the intention of this patch. The new patch would basically be Diff3 of this patch.

We should think about optimizing Function::hasAddressTaken() by introducing a bool in Function class which would cache the result. In addition to this, Function::hasAddressTaken() can accept an additional bool parameter and based on its truthness, the function can return the cached value. The default value of parameter bool should be false to preserve today's behavior but client can set it to "true" if they want the cached value. Latter would be useful for this patch as the information is unlikely to change in a module (which is static naturally). This optimization would significantly improve the run-time of hasAddressTaken().

Without this change, internal ci takes ~10 minutes on average to compile rocThrust, ~9 minutes on average to compile hipCUB.

With this change, internal ci timed out after compiling rocThrust for 1.5 hours, timed out after compiling hipCUB after 1 hour.

When bisecting on my local machine, I gave 1 hour on top of what ci gave before terminating...

It did *not* look like a 1-2% increase in time.

[madhur13490]

In D94585#2524490, @kzhuravl wrote:

In D94585#2524476, @madhur13490 wrote:

In D94585#2520966, @kzhuravl wrote:

This change is causing infinite loop when compiling rocThrust and hipCUB. Can you take a look? Thanks.

FWIW, after testing rocThrust and hipCUB, it turned out that, it is not an infinite loop but a rise in compile-time which crossed timing threshold of the internal infra. The apps eventually compiled with 1-2% increment in the compile-time. I figured out the cause behind this. This patch makes two additional calls to Function::hasAddressTaken() and hasAddressTaken() is not optimal. Each time Function::hasAddressTaken() is called, it traverses over all uses of the function to deduce the required info. The calls made by this patch are itself in the loop which effectively made the suboptimality of Function::hasAddressTaken() conspicuous. In the new patch I am going to remove one call to Function::hasAddressTaken() which still preserves the intention of this patch. The new patch would basically be Diff3 of this patch.

We should think about optimizing Function::hasAddressTaken() by introducing a bool in Function class which would cache the result. In addition to this, Function::hasAddressTaken() can accept an additional bool parameter and based on its truthness, the function can return the cached value. The default value of parameter bool should be false to preserve today's behavior but client can set it to "true" if they want the cached value. Latter would be useful for this patch as the information is unlikely to change in a module (which is static naturally). This optimization would significantly improve the run-time of hasAddressTaken().

Without this change, internal ci takes ~10 minutes on average to compile rocThrust, ~9 minutes on average to compile hipCUB.

With this change, internal ci timed out after compiling rocThrust for 1.5 hours, timed out after compiling hipCUB after 1 hour.

When bisecting on my local machine, I gave 1 hour on top of what ci gave before terminating...

It did *not* look like a 1-2% increase in time.

Well, when I tried locally in the CI's container, it did not take more than 20mins to compile so I am not sure why it was stuck for more 1-2 hours.

This is what I got from -time-passes of llc.

W/o my change:
8.1618 ( 1.1%) 0.0000 ( 0.0%) 8.1618 ( 1.1%) 8.1637 ( 1.1%) Early propagate attributes from kernels to functions

W/ my change:
9.8957 ( 1.4%) 0.0130 ( 0.3%) 9.9088 ( 1.4%) 9.9124 ( 1.4%) Early propagate attributes from kernels to functions

[rampitec]

Before we continue to solutions could you please clarify what in the current rocThrust has address taken? It may not have indirect function calls and without it this change should be a no-op.

llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp

bool AMDGPUPropagateAttributes::process() {
   do {
     Roots.insert(NewRoots.begin(), NewRoots.end());
     NewRoots.clear();
 
     for (auto &F : M.functions()) {
       if (F.isDeclaration())
         continue;
 
+      if (F.hasAddressTaken()) {
+        // Skip propagating attributes to address taken functions
+        // and just set features.
+        auto Features = TM->getSubtargetImpl(F)->getFeatureBits();
+        FeatureBitset FB(Features & ~TargetFeatures);
+        setFeatures(F, FB);
+        continue;
+      }
+
       const FnProperties CalleeProps(*TM, F);
       SmallVector<std::pair<CallBase *, Function *>, 32> ToReplace;
       SmallSet<CallBase *, 32> Visited;
 
       for (User *U : F.users()) {
         Instruction *I = dyn_cast<Instruction>(U);
         if (!I)
           continue;
         CallBase *CI = dyn_cast<CallBase>(I);
         if (!CI)
           continue;
         Function *Caller = CI->getCaller();
         if (!Caller || !Visited.insert(CI).second)
           continue;
         if (!Roots.count(Caller) && !NewRoots.count(Caller))
           continue;
 
         const FnProperties CallerProps(*TM, *Caller);
 
         if (CalleeProps == CallerProps) {
           if (!Roots.count(&F))
             NewRoots.insert(&F);
           continue;
         }
 
         Function *NewF = findFunction(CallerProps, &F);
         if (!NewF) {
           const FnProperties NewProps = CalleeProps.adjustToCaller(CallerProps);
           if (!AllowClone) {
             // This may set different features on different iteartions if
             // there is a contradiction in callers' attributes. In this case
             // we rely on a second pass running on Module, which is allowed

llvm/test/CodeGen/AMDGPU/propagate-attributes-direct-indirect.ll

+; RUN: opt -S -mtriple=amdgcn-amd-amdhsa -amdgpu-propagate-attributes-early %s | FileCheck %s
+
+target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-G1-ni:7"
+target triple = "amdgcn-amd-amdhsa"
+
+; CHECK: define hidden float @_Z3fooi(i32 %a) #0 {
+define hidden float @_Z3fooi(i32 %a) {
+entry:
+  %mul = mul nsw i32 %a, 5
+  %cast = sitofp i32 %mul to float
+
+  ret float %cast
+}
+
+; CHECK: define hidden float @_Z3bari(i32 %a) #0 {
+define hidden float @_Z3bari(i32 %a) {
+entry:
+  %div = sdiv i32 %a, 7
+  %conv = sitofp i32 %div to float
+  ret float %conv
+}
+
+; CHECK: define hidden float @_Z3bazi(i32 %a) #0 {
+define hidden float @_Z3bazi(i32 %a) {
+entry:
+  %mul = mul nsw i32 %a, 6
+  %div = sdiv i32 %mul, 7
+  %conv = sitofp i32 %div to float
+  ret float %conv
+}
+
+; CHECK: define hidden float @_Z4baz2i(i32 %a) #1 {
+define hidden float @_Z4baz2i(i32 %a) #0 {
+  %mul = mul nsw i32 %a, 6
+  %div = sdiv i32 %mul, 8
+  %add = add i32 %div , 12
+  %conv = sitofp i32 %add to float
+  ret float %conv
+}
+
+; CHECK: define protected amdgpu_kernel void @_Z22switch_indirect_kernelPfi(float addrspace(1)* %result.coerce, i32 %type) #2 {
+define protected amdgpu_kernel void @_Z22switch_indirect_kernelPfi(float addrspace(1)* %result.coerce, i32 %type) #1 {
+entry:
+  %result = alloca float*, align 8, addrspace(5)
+  %result.ascast = addrspacecast float* addrspace(5)* %result to float**
+  %result.addr = alloca float*, align 8, addrspace(5)
+  %result.addr.ascast = addrspacecast float* addrspace(5)* %result.addr to float**
+  %type.addr = alloca i32, align 4, addrspace(5)
+  %type.addr.ascast = addrspacecast i32 addrspace(5)* %type.addr to i32*
+  %fn = alloca float (i32)*, align 8, addrspace(5)
+  %fn.ascast = addrspacecast float (i32)* addrspace(5)* %fn to float (i32)**
+  %P = alloca float, align 4, addrspace(5)
+  %P.ascast = addrspacecast float addrspace(5)* %P to float*
+  %0 = addrspacecast float addrspace(1)* %result.coerce to float*
+  store float* %0, float** %result.ascast, align 8
+  %result1 = load float*, float** %result.ascast, align 8
+  store float* %result1, float** %result.addr.ascast, align 8
+  %1 = bitcast float (i32)* addrspace(5)* %fn to i8 addrspace(5)*
+  %2 = load i32, i32* %type.addr.ascast, align 4
+  switch i32 %2, label %sw.default [
+    i32 1, label %sw.bb
+    i32 2, label %sw.bb2
+    i32 3, label %sw.bb3
+  ]
+
+sw.bb:                                            ; preds = %entry
+  store float (i32)* @_Z3fooi, float (i32)** %fn.ascast, align 8
+  br label %sw.epilog
+
+sw.bb2:                                           ; preds = %entry
+  store float (i32)* @_Z3bari, float (i32)** %fn.ascast, align 8
+  br label %sw.epilog
+
+sw.bb3:                                           ; preds = %entry
+  store float (i32)* @_Z3bazi, float (i32)** %fn.ascast, align 8
+  br label %sw.epilog
+
+sw.default:                                       ; preds = %entry
+  br label %sw.epilog
+
+sw.epilog:                                        ; preds = %sw.default, %sw.bb3, %sw.bb2, %sw.bb
+  %fp = load float (i32)*, float (i32)** %fn.ascast, align 8
+  %direct_call = call contract float @_Z4baz2i(i32 5)
+  %conv = fptosi float %direct_call to i32
+  %call4 = call contract float %fp(i32 %conv)
+  %res = load float*, float** %result.addr.ascast, align 8
+  store float %call4, float* %res, align 4
+  ret void
+}
+
+attributes #1 = { convergent norecurse nounwind mustprogress "amdgpu-flat-work-group-size"="1,256" "amdgpu-implicitarg-num-bytes"="56" "amdgpu-unsafe-fp-atomics"="true" "disable-tail-calls"="false" "frame-pointer"="none" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx906" "target-features"="+16-bit-insts,+ci-insts,+dl-insts,+dot1-insts,+dot2-insts,+dpp,+flat-address-space,+gfx8-insts,+gfx9-insts,+s-memrealtime" "uniform-work-group-size"="true" "unsafe-fp-math"="false" "use-soft-float"="false" }

llvm/test/CodeGen/AMDGPU/propagate-attributes-indirect.ll

+; RUN: opt -S -mtriple=amdgcn-amd-amdhsa -amdgpu-propagate-attributes-early %s | FileCheck %s
+
+target datalayout = "e-p:64:64-p1:64:64-p2:32:32-p3:32:32-p4:64:64-p5:32:32-p6:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-S32-A5-G1-ni:7"
+target triple = "amdgcn-amd-amdhsa"
+
+; CHECK: define hidden float @_Z3fooi(i32 %a) #0 {
+define hidden float @_Z3fooi(i32 %a) {
+entry:
+  %mul = mul nsw i32 %a, 5
+  %cast = sitofp i32 %mul to float
+
+  ret float %cast
+}
+
+; CHECK: define hidden float @_Z3bari(i32 %a) #0 {
+define hidden float @_Z3bari(i32 %a) {
+entry:
+  %div = sdiv i32 %a, 7
+  %conv = sitofp i32 %div to float
+  ret float %conv
+}
+
+; CHECK: define hidden float @_Z3bazi(i32 %a) #0 {
+define hidden float @_Z3bazi(i32 %a) {
+entry:
+  %mul = mul nsw i32 %a, 6
+  %div = sdiv i32 %mul, 7
+  %conv = sitofp i32 %div to float
+  ret float %conv
+}
+
+; CHECK: define protected amdgpu_kernel void @_Z22switch_indirect_kernelPfi(float addrspace(1)* %result.coerce, i32 %type) #1 {
+define protected amdgpu_kernel void @_Z22switch_indirect_kernelPfi(float addrspace(1)* %result.coerce, i32 %type) #1 {
+entry:
+  %result = alloca float*, align 8, addrspace(5)
+  %result.ascast = addrspacecast float* addrspace(5)* %result to float**
+  %result.addr = alloca float*, align 8, addrspace(5)
+  %result.addr.ascast = addrspacecast float* addrspace(5)* %result.addr to float**
+  %type.addr = alloca i32, align 4, addrspace(5)
+  %type.addr.ascast = addrspacecast i32 addrspace(5)* %type.addr to i32*
+  %fn = alloca float (i32)*, align 8, addrspace(5)
+  %fn.ascast = addrspacecast float (i32)* addrspace(5)* %fn to float (i32)**
+  %P = alloca float, align 4, addrspace(5)
+  %P.ascast = addrspacecast float addrspace(5)* %P to float*
+  %0 = addrspacecast float addrspace(1)* %result.coerce to float*
+  store float* %0, float** %result.ascast, align 8
+  %result1 = load float*, float** %result.ascast, align 8
+  store float* %result1, float** %result.addr.ascast, align 8
+  %1 = bitcast float (i32)* addrspace(5)* %fn to i8 addrspace(5)*
+  %2 = load i32, i32* %type.addr.ascast, align 4
+  switch i32 %2, label %sw.default [
+    i32 1, label %sw.bb
+    i32 2, label %sw.bb2
+    i32 3, label %sw.bb3
+  ]
+
+sw.bb:                                            ; preds = %entry
+  store float (i32)* @_Z3fooi, float (i32)** %fn.ascast, align 8
+  br label %sw.epilog
+
+sw.bb2:                                           ; preds = %entry
+  store float (i32)* @_Z3bari, float (i32)** %fn.ascast, align 8
+  br label %sw.epilog
+
+sw.bb3:                                           ; preds = %entry
+  store float (i32)* @_Z3bazi, float (i32)** %fn.ascast, align 8
+  br label %sw.epilog
+
+sw.default:                                       ; preds = %entry
+  br label %sw.epilog
+
+sw.epilog:                                        ; preds = %sw.default, %sw.bb3, %sw.bb2, %sw.bb
+  %fp = load float (i32)*, float (i32)** %fn.ascast, align 8
+  %call4 = call contract float %fp(i32 6)
+  %res = load float*, float** %result.addr.ascast, align 8
+  store float %call4, float* %res, align 4
+  ret void
+}
+
+attributes #1 = { convergent norecurse nounwind mustprogress "amdgpu-flat-work-group-size"="1,256" "amdgpu-implicitarg-num-bytes"="56" "amdgpu-unsafe-fp-atomics"="true" "disable-tail-calls"="false" "frame-pointer"="none" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="gfx906" "target-features"="+16-bit-insts,+ci-insts,+dl-insts,+dot1-insts,+dot2-insts,+dpp,+flat-address-space,+gfx8-insts,+gfx9-insts,+s-memrealtime" "uniform-work-group-size"="true" "unsafe-fp-math"="false" "use-soft-float"="false" }