[PATCH] D101847: [AMDGPU] Fix function pointer argument bug in AMDGPU Propagate Attributes pass.

Matt Arsenault via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue May 4 17:41:44 PDT 2021


arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPropagateAttributes.cpp:252
         CallBase *CI = dyn_cast<CallBase>(I);
-        if (!CI)
+        if (!CI || CI->getCalledFunction() != &F)
           continue;
----------------
jweightman wrote:
> arsenm wrote:
> > jweightman wrote:
> > > arsenm wrote:
> > > > What if the callee was a bitcast of the function? Can you add another testcase with a constexpr function pointer cast?
> > > I tried adding this to the LIT test:
> > > ```
> > >     %h_cast = bitcast i32* ()* @h to i8* ()*
> > >     %1 = call i8* %h_cast()
> > > ...
> > > define private i32* @h() #1 {
> > >     %1 = alloca i32
> > >     ret i32* %1
> > > }
> > > ```
> > > 
> > > and the pass does not propagate the attributes like one would probably want. However, this seems pretty orthogonal to the problem I was trying to fix (preventing the illegal callee change), because the "user" is an `llvm::BitCastInstr` so `CI` is always `nullptr` in that case. 
> > > 
> > > In order to handle bitcasts, I think we would need new logic to detect which bitcasted functions need to be replaced, a new data structure to track them in (similar to the existing `ToReplace`), and new logic to do that replacement — all of which would be on a separate logical branch from the existing code for handling call instructions! Unless you see an easier way, I think such a change is beyond the scope of what I was trying to do.
> > This example isn't a constant expression cast. If you run opt -S -instcombine on this, it should turn it into the constant expr cast form. In that case the user will still be CallInst, it's just CI->getCalledFunction() will be a constantexpr cast of the function. stripPointerCasts should get you the base function
> I see. I misunderstood your initial comment. This seems like a reasonable thing to try to support with this diff.
> 
> The instcombine pass seems to prefer to cast the argument/return value (I modified the test to try both) rather than the function pointer itself. Going back to the IR Language Reference, I saw the constant expression section (I'm still new at this...), so I tried this IR:
> ```
> %1 = alloca i8
> %2 = call i8* bitcast (i32* ()* @h to i8* ()*)()
> ```
> However, the Propagate Attributes pass doesn't produce a clone of `@h` in this situation. I see that the `dyn_cast<Instruction>(U)` fails because the user of `h` is just the constexpr bitcast:
> ```
> > U->dump()
> void (i8*)* bitcast (void (i32*)* @g to void (i8*)*)
> ```
> Sorry for the back and forth, but is there some way to find an enclosing `CallBase` if it exists? 
I don't think there's a super easy way to do it. You would have to find the context. The normal way to check that would be to start from the instruction and look into the constantexpr for the global, which starts looking like general indirect call handling.

Ultimately this pass is a grotesque hack that should absolutely not exist, and only does because comgr still does not link the device libraries correctly for opencl. How did you run into this?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101847/new/

https://reviews.llvm.org/D101847



More information about the llvm-commits mailing list