Fix `ExtractMemberOp` and `InsertMemberOp` constraints

[bcardosolopes]

Seems like it's supposed to only get records but we are also passing methods for it (introduced in https://github.com/llvm/clangir/pull/1292). @Lancern can you clarify the intent?

void ItaniumCXXABI::lowerGetMethod(
  ...
  mlir::Value &callee = loweredResults[0];
  mlir::Value &adjustedThis = loweredResults[1];
  mlir::Type calleePtrTy = op.getCallee().getType();

  cir::IntType ptrdiffCIRTy = getPtrDiffCIRTy(LM);
  mlir::Value ptrdiffOne = rewriter.create<cir::ConstantOp>(
      op.getLoc(), cir::IntAttr::get(ptrdiffCIRTy, 1));

  mlir::Value adj = rewriter.create<cir::ExtractMemberOp>(
      op.getLoc(), ptrdiffCIRTy, loweredMethod, 1);

[bcardosolopes]

@xlauko I think we can relax the constraints for now while we have some discussion with @Lancern

[bcardosolopes]

Redesigning the conversion pattern to use different operations could be an option, though I haven't got deep into the details of this just yet.

[Lancern]

Thanks for pointing this out @bcardosolopes .

The constraint of ExtractMemberOp here does not apply because the operation is "temporary" -- it is created during the LLVMIR lowering pass, and then it immediately gets lowered to LLVMIR in the same pass. This is how pattern rewrite pass works: it recursively and repeatedly applies pattern rewrites on the IR until no patterns can be matched. IR verification only happens between passes and never during a pass (unless you explicitly require so), thus no verification errors are produced. No matter what type of values are passed to ExtractMemberOp, eventually when the LLVMIR lowering finishes they will all be lowered to LLVMIR dialect and the code just works.

The ExtractMemberOp here is trying to get the internal fields of a !cir.method value, which is actually a !cir.struct with two fields in Itanium ABI. The lowering path !cir.method -> !cir.struct -> !llvm.struct happens simultaneously with the lowering of cir.get_method and this indeed creates some level of confusion.

I'm thinking maybe we could add a new pass before CallConv lowering that does this sort of ABI lowering job. This pass should convert all appearances of !cir.method to !cir.struct, and lower cir.get_method to a sequence of more "basic" ops including cir.extract_member. This way we don't have to deal with a soup of ABI lowering code together with LLVMIR lowering code inside one pass.

[bcardosolopes]

The ExtractMemberOp here is trying to get the internal fields of a !cir.method value, which is actually a !cir.struct with two fields in Itanium ABI. The lowering path !cir.method -> !cir.struct -> !llvm.struct happens simultaneously with the lowering of cir.get_method and this indeed creates some level of confusion.

Gotcha, thanks for the clarification.

I'm thinking maybe we could add a new pass before CallConv lowering that does this sort of ABI lowering job. This pass should convert all appearances of !cir.method to !cir.struct, and lower cir.get_method to a sequence of more "basic" ops including cir.extract_member. This way we don't have to deal with a soup of ABI lowering code together with LLVMIR lowering code inside one pass.

Maybe lowering prepare is a good candidate or is to early for that?

[Lancern]

Maybe lowering prepare is a good candidate or is to early for that?

We can put the lowering of !cir.method at lowering prepare and it will also work, but we still have the same problem. Lowering prepare is not dedicated to ABI lowering, it contains code for non-ABI stuff. We're still mixing ABI lowering code and non-ABI lowering code together if we put !cir.method lowering there.

The tricky thing about !cir.method is that we need to ABI-lower a type rather than a single operation. AFAIK, MLIR does not have direct support for automatically lowering a type: to lower a type, all operations that involve an operand of that type need to be rewritten manually as well. Thus, to properly ABI-lower !cir.method, we have to write multiple rewrite patterns that rewrite multiple operations in a single pass, including those seemingly non-ABI-relevant operations such as cir.load and cir.store. Thus I believe it could be more elegant to collect these special "ABI-lowering" rewrite patterns into a new pass that is dedicated for the ABI lowering job.

[xlauko]

The mechanism to do that is Source Materialization in type rewriter, which essentially materializes to unrealized casts if no there is no pattern that fixes users.

My general problem with current approach is that it has weird typing. I would expect!cir.method to be some sort of pointer to struct or its offset, so it does not really make sense to extract/insert to it.

[xlauko]

I would expect lowering path to be something that mirrors pointer semantic of method more, ie.:

!cir.method translater to !cir.ptr<!cir.struct> + some pointer arithmetic.

Or maybe some intermediate operation that gets !cir.struct from !cir.method might do the trick, but I also haven't had time to explore the conversion more. In general it feels wrong to allow Extract and Insert, which are "value" operations, to operate on !cir.method which is "pointer", though cir.load also feels wrong.

[bcardosolopes]

Or maybe some intermediate operation that gets !cir.struct from !cir.method might do the trick, but I also haven't had time to explore the conversion more. In general it feels wrong to allow Extract and Insert, which are "value" operations, to operate on !cir.method which is "pointer", though cir.load also feels wrong.

+1