Fix invalid LLVM IR by updating abitype and buildVal

[zhouguangyuan0718]

1. use NamedStruct in abitype
2. use aggregateValue instead of ConstArray in buildVal
3. do not create alloca in non-entry block

[gemini-code-assist]

Code Review

This pull request introduces a createEntryAlloca method to ensure that alloca instructions are consistently placed in the function's entry block, which facilitates better LLVM optimizations like SROA and mem2reg. The changes refactor the SSA builder to use this method and update test expectations. Feedback was provided to refine the createEntryAlloca implementation for better consistency, efficiency, and safety, specifically suggesting a unified insertion logic and proper handling of empty entry blocks.

[gemini-code-assist]

The implementation of createEntryAlloca has several issues that affect both compiler efficiency and the quality of the generated IR:

1. Inconsistency: The current logic appends the alloca to the entry block if the builder is already positioned there (lines 162-163 and 167-168), but prepends it to the start of the entry block if called from a different block (lines 171-173). For optimal LLVM optimization (such as SROA and mem2reg), it is a best practice to place all alloca instructions at the very beginning of the entry block.
2. Redundancy: The checks b.blk.Index() == 0 and blk == entry are redundant as they both attempt to identify if the current insertion point is within the entry block.
3. Efficiency: Creating and disposing of a new llvm.Builder on every call (lines 171-174) adds unnecessary overhead to the compiler's execution. While a dedicated builder could be cached, at the very least, the logic should be unified.
4. Safety: entry.FirstInstruction() might return a nil value if the block is empty (though unlikely in a valid function body), which could cause SetInsertPointBefore to fail. It is safer to check for this or use SetInsertPointAtEnd as a fallback.

Consider a unified approach that always ensures the alloca is placed at the start of the entry block for consistency.

func (b Builder) createEntryAlloca(typ llvm.Type) llvm.Value {
	entry := b.Func.impl.EntryBasicBlock()
	entryB := b.Prog.ctx.NewBuilder()
	defer entryB.Dispose()
	if first := entry.FirstInstruction(); !first.IsNil() {
		entryB.SetInsertPointBefore(first)
	} else {
		entryB.SetInsertPointAtEnd(entry)
	}
	return llvm.CreateAlloca(entryB, typ)
}

[fennoai]

Review Summary

Solid PR that fixes three classes of invalid LLVM IR. The changes are well-scoped and the new tests directly verify both the presence of correct patterns and absence of incorrect ones. A few observations below, mostly around consistency and robustness — nothing blocking.

Changes reviewed:

• ConstNamedStruct in abitype (correct fix for named struct type mismatch)
• aggregateValue replacing ConstArray in buildVal (correct — ConstArray requires constant elements)
• Entry-block alloca hoisting via createEntryAlloca (standard LLVM best practice)
• Test fixes using ConstPtrToInt (valid IR constants)

[fennoai]

Consistency question: AllocaT here still uses llvm.CreateAlloca(b.impl, t.ll) directly. It's called from di.go:698 which could potentially be invoked from non-entry blocks during debug info emission. Similarly, Alloca (line 144) and ArrayAlloca (line 286) use raw CreateAlloca/CreateArrayAlloca.

Is this an intentional scoping decision (only fixing the call sites that actually triggered invalid IR), or should these be updated too? If intentional, a brief comment explaining why they're exempt would help future maintainers.

[fennoai]

Consider adding a doc comment explaining the LLVM constraint this method addresses, e.g.:

Suggested change

	func (b Builder) createEntryAlloca(typ llvm.Type) llvm.Value {
	entry := b.Func.impl.EntryBasicBlock()
	entryB := b.Prog.ctx.NewBuilder()
	defer entryB.Dispose()
	// createEntryAlloca places an alloca in the function's entry block.
	// LLVM requires allocas to be in the entry block for reliable stack
	// frame layout and mem2reg promotion to SSA registers.
	func (b Builder) createEntryAlloca(typ llvm.Type) llvm.Value {

Also, a brief inline comment explaining why both the SSA-level check (b.blk.Index() == 0) and the LLVM-level check (blk == entry) are needed would help — they can diverge when SetBlockEx is called with setBlk=false.

[fennoai]

Minor robustness note: if this is ever called before any instruction is emitted in the entry block, entry.FirstInstruction() could return a nil value. In practice the guard at line 162 (b.blk == nil || b.blk.Index() == 0) short-circuits the common case, and existing code in internal/cabi/cabi.go uses the same pattern, so this is consistent — just worth noting the invariant.

Also: creating and disposing a new builder per call is fine given the bounded number of allocas per function, but if this ever becomes a hot path, caching a secondary builder on the builder struct would avoid repeated CGo roundtrips.

[fennoai]

Good fix. ConstArray requires constant elements, which isn't guaranteed in the type-assertion path. Using aggregateValue (insertvalue chain from undef) is the correct approach for runtime values.

Nit: buildVal always wraps a single element per recursion level (field 0 for structs, element 0 for arrays). A brief comment explaining this invariant would help readers understand why only one element is passed to aggregateValue.