skc7 wrote:
> > > Can you tell me more about how you intend to use this? This is a very
> > > low-level representation. I'd prefer to have less direct coupling with
> > > the LLVM IR form in CIR.
> >
> >
> > Thanks Andy. The use case is lowering AMDGPU builtins. Several of them
> > lower to LLVM intrinsics that take a metadata operand, e.g. the
> > `sync-scope` string for
> > [cooperative_atomic_{load,store}_*](https://github.com/llvm/llvm-project/blob/72b891b75f85/clang/lib/CodeGen/TargetBuiltins/AMDGPU.cpp#L1000-L1011),
> >
> > [global/flat_load_monitor_*](https://github.com/llvm/llvm-project/blob/72b891b75f85/clang/lib/CodeGen/TargetBuiltins/AMDGPU.cpp#L935-L937)
> > etc. CIRGen emits these through `cir.call_llvm_intrinsic`, so it needs a
> > way to materialize a metadata value to pass as the operand.
> > Looking at other targets' CodeGen and CIR's architecture, the dialect is
> > designed to be self-contained. It doesn't reference LLVM-dialect specifics
> > directly, so it needs its own representation that lowers to them. That's
> > all this PR adds: `!cir.metadata` / `#cir.md_string` / `#cir.md_node` /
> > `cir.metadata_as_value`, which mirror the LLVM dialect's metadata
> > constructs purely so CIR can carry the operand and lower it to the LLVM
> > dialect 1:1. They hold no CIR-level semantics; they exist only to feed
> > intrinsic calls.
>
> I'd prefer to see these things modeled at a higher level as much as possible.
> I realize that this may become a problem as we get deeper into
> target-specific intrinsics, but such intrinsics make the IR somewhat opaque
> to target-independent transformations. We already have attributes for
> expressing sync-scope on load/store operations. Could you add a `cooperative`
> attribute to load and store and defer the intrinsic call creation until
> lowering to LLVM IR?
>
> There was a similar push years ago in LLVM IR, where we attempted to lower
> source-level target intrinsic calls to target-independent representations.
> We've drifted from that goal a bit as more and more targets have been added,
> but I think target-independent IR should still be the ideal that we strive
> for.
>
> For cases where extending general operations like load/store isn't practical
> but the underlying intrinsic requires a metadata argument (and
> global/flat_load_monitor may be such a case), we could introduce
> target-specific CIR operations, for example perhaps `cir.amdgcn.load_monitor`?
>
> I just really don't want to introduce an element in CIR that directly maps to
> an LLVM IR construct. Tight-coupling with LLVM IR defeats the purpose of MLIR
> dialects.
Thanks @andykaylor for feedback. I will try to introduce attributes to load or
a new amdgcn cir op as per requirement in builtins codegen.
https://github.com/llvm/llvm-project/pull/204190
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
