[Mlir-commits] [mlir] [mlir][spirv] Add convergent attribute to builtin (PR #122131)

[Victor Perez]

victor-eds wrote:

> > I see what @kuhar means. Maybe simply doing:
> > ```c++
> >     LLVM::LLVMFuncOp func = lookupOrCreateSPIRVFn(
> >         symbolTable, funcName, paramTypes, retTy, /*isConvergent=*/true);
> > ```
> > 
> > 
> >     
> >       
> >     
> > 
> >       
> >     
> > 
> >     
> >   
> > and keeping `lookupOrCreateSPIRVFn`'s definition as is is better.
> 
> Yeah, sorry for misunderstanding your point yesterday @kuhar.
> 
> I had only introduced the `convergent` parameter to `lookupOrCreateSPIRVFn` function in the previous PR to add lowering for group operations here: #115501
> 
> The use of this function that I changed in this PR was the only use of that function that specifies a value for `convergent`.
> 
> The only other use of of `lookupOrCreateSPIRVFn` already unconditionally sets `true` through the default value: https://github.com/llvm/llvm-project/blob/main/mlir/lib/Conversion/SPIRVToLLVM/SPIRVToLLVM.cpp#L1080
> 
> That's why I figured I could remove the `convergent` parameter again.
> 
> However, if you and @victor-eds prefer to keep the parameter, I can bring it back, no problem.
> 
> WDYT?

I'd rather keep the option as we may have to bring it back again in the future.

https://github.com/llvm/llvm-project/pull/122131