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

[Jacob Weightman via Phabricator via llvm-commits]

jweightman 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;
----------------
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? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101847