[CIR]fix cir.std.initializer_list validate issue for struct type initlist

[HerrCai0907]

cir test is missing because StructType::getPreferredAlignment is still NYI

[seven-mile]

Better be careful here. What about array type?

[seven-mile]

Thanks for fixing it. Array type is an apparent one of the cases. I don't know how even complex types (e.g. those introduced by exceptions) should work, and what we should do to maintain the code when extending the type system of CIR. Options could be like, collecting predicates in one single place or adding a FIXME here.

Sorry for blocking it. The changes may not necessarily be included in this patch. You can wait @bcardosolopes for his advice ; )

[HerrCai0907]

I also feel uncomfortable for this code. The reason is in CIR, we actually have 2 kinds of value, one pass by value, the other pass by pointer. Could we somehow align them?
the full support for cir.std.initializer_list is still on going. And I am confused how to handle this two kinds. Especially for the struct with deconstruct function. The lifetime of temporary variable will be hard to manage.

[seven-mile]

IMHO, this is another orthogonal design choice to retain some low-level constructs, such as pointers, in function calls. It's worth noting that CIR is currently somewhat low-level by design, as gradual improvements are planned. There are also many discussion on it in progress, e.g. this one that you may be interested in ; )

In the last patch for cir.std.init_list, I believe that we essentially agreed on treating this op as something akin to a "macro" that organizes inputs more effectively while maintaining a clear correspondence to the original low-level ops (introduced in #764). Do you think such approach has any drawbacks?

[HerrCai0907]

IMHO, this is another orthogonal design choice to retain some low-level constructs, such as pointers, in function calls. It's worth noting that CIR is currently somewhat low-level by design, as gradual improvements are planned. There are also many discussion on it in progress, e.g. this one that you may be interested in ; )

Maybe we need a clearer guideline to point which type will pass by ptr. e.g. most of struct is passed by pointer. But cir.std.initializer_list will create a struct directly. What will happen if we want to construct a initializer_list of initializer_list?

[seven-mile]

What will happen if we want to construct a initializer_list of initializer_list?

Indeed. Furthermore, we probably have some unfortunate miscompilations here. This godbolt shows an example. (Godbolt still holds the old version of ClangIR with #764, the old approach).

%1 = cir.alloca ... ["sub_list"] // alloca of inner sub_list
%tmp.arg.array = cir.alloca !cir.array<!ty_std3A3Ainitializer_list3CA3E x 4>
... // initialization of sub_list
%9 = cir.ptr_stride(%tmp.arg.array.addr, ...) // some pointer arithmetic on init list buffer
cir.call @_ZNSt16initializer_listI1AEC2ERKS1_(%9, %1) // call the copy ctor to initialize the element

Note that we are allocating an array of struct as a buffer, so we are always manipulating pointers. The copy ctor is accomplished in place. This is from the following lines from OG CodeGen:

clangir/clang/lib/CodeGen/CGExprAgg.cpp

Lines 1777 to 1783 in 888f00c

	if (curInitIndex < NumInitElements) {
	// Store the initializer into the field.
	EmitInitializationToLValue(InitExprs[curInitIndex++], LV);
	} else {
	// We're out of initializers; default-initialize to null
	EmitNullInitializationToLValue(LV);
	}

For the new approach, we emit intermediate value tmpInit in CIRGen, which is the inputs of cir.std.initializer_list.

// tmpInit emitted in CIRGen by emitAnyExprToTemp
%4 = cir.alloca !ty_std3A3Ainitializer_list3CA3E, ["agg.tmp4"]
cir.call @_ZNSt16initializer_listI1AEC1ERKS1_(%4, %sub_list.addr) // copy the sub_list to tmpInit (aka. args[2])
... // omitted
%16 = cir.ptr_stride(%tmp.arg.array.addr, #2) // buffer[2]
// wrong bitcast produced in LoweringPrepare by createStore
%17 = cir.cast(bitcast, %16: !cir.ptr<!ty_std3A3Ainitializer_list3CA3E>), !cir.ptr<!cir.ptr<!ty_std3A3Ainitializer_list3CA3E>>
// !!! we store a pointer to element to the placeholder of the element, which is wrong
cir.store %4, %17 : !cir.ptr<!ty_std3A3Ainitializer_list3CA3E>, !cir.ptr<!cir.ptr<!ty_std3A3Ainitializer_list3CA3E>>

Here cir.store leads to miscompilation, associated with these lines:

clangir/clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp

Lines 1159 to 1168 in 888f00c

	for (unsigned i = 0; i < args.size(); i++) {
	if (i == 0) {
	builder.createStore(loc, args[i], arrayStartPtr);
	} else {
	mlir::Value offset = builder.getUnsignedInt(loc, i, 64);
	mlir::Value dest = builder.create<cir::PtrStrideOp>(
	loc, arrayStartPtr.getType(), arrayStartPtr, offset);
	builder.createStore(loc, args[i], dest);
	}
	}

---

        AST                       CodeGen                     LLVM IR
Children of InitListExpr --[EmitInitializationToLValue]--> Temporary Array

        AST                       CIRGen                                       LoweringPrepare     Low-level CIR
Children of InitListExpr --[emitAnyExprToTemp]--> tmpInit (inputs of the op) --[     ???     ]--> Temporary Array

The key difference here is that we inserts an extra "move" operation when AST does not represent one. And it's not properly implemented now (it's a direct createStore). Apart from that, the design of intermediate values should not be blamed. If we need collect all the inputs, we have to represent these temporary values anyway.

So what's the solution? I agree that we probably need some kind of value semantics to flawlessly represent std::initializer_list, so that we don't need to care about how an RValue is stored in memory. The current design of cir.std.initializer_list might be a bit premature advanced for ClangIR. But it does not seems to be a so fatal issue that we won't find any workaround... For example, AST back references.

Bothering calls to @bcardosolopes @smeenai for their opinions. 😂

[HerrCai0907]

sorry, i confused by some outdated design which contains alloca op in std.initlist also.

[HerrCai0907]

If we distinguish cir type with 2 kinds. one is by ptr, the other is by value. the current design IMO is fine. it is not so harmful to support two kinds of input.

for value type, we must store it to array. it is the same as current llvmir.
for struct/array, construct a tmp rvalue and move it to array IMO also fulfill standard and can be optimized later.
it still remains some questions, e.g. when to deconstruct tmp value. i am trying to find a solution now.

[smeenai]

Sorry for the late reply. Last week was Thanksgiving week in the US, and I fell behind afterwards.

This is an interesting case that I didn't think about at all during the original review; thanks for noticing it and putting up the PR. I agree that the fundamental fix would be having value semantics. Doing the value vs. pointer type split might work for now, but it feels fragile. I know @bcardosolopes had some thoughts here too; he's traveling right now but he can weigh in when he gets the chance.

Slightly unrelated, but I'm working on compiler-explorer/clang-builder#75 to get us a more up-to-date ClangIR compiler on Godbolt.

[bcardosolopes]

@HerrCai0907 I was giving this more thought, and I think we should take a step back:

• We should revert the PR that introduced cir.std.initializer_list.
• Introduce a new PR where cir.std.initializer_list is just a thin layer over the initialization work, for example:

%0 = cir.alloca INITLIST_TYPE, !cir.ptr<INITLIST_TYPE>
cir.std.initializer_list {
  %1 = cir.const #cir.int<1>
  ... // all init work goes here
}

My rationale is that we don't yet have a pass taking advantage of this and we should probably use that to guide how we really want to design this, anything before that will be more or less a guess game. Once we have usage going through it and we start writing a pass that wants to look at the initialization data, then we refine. Sorry for the churn, WDYT?