Fix rewriteValueUsesToAddrUses and add regression test

[skiminki-nv]

Function rewriteValueUsesToAddrUses() translates
IRFieldExtract/IRGetElement/IRStore instructions that are affected by the change of the function parameter type from value to pointer. However, the implementation failed to consider the case where:

• The base of translated IRGetElement() is IRFieldExtract(); and
• The index of translated IRGetElement() is IRFieldExtract() or another IRGetElement()
  In the problem case, the index would be passed a pointer instead of a dereferenced pointer.

To fix this, rewrite the function with a slightly different strategy:

• Start by injecting IRLoad() to all uses of the transformed function parameter.
• Then, follow up with the injected IRLoad() ops:
  • Transform IRFieldExtract(IRLoad(ptr), ...) and IRGetElement(IRLoad(ptr), ...) patterns to IRLoad(IRFieldAddress(ptr, ...)) and IRLoad(IRGetElementPtr(ptr, ...)), and keep following up with the newly injected IRLoad() ops.
  • Transform also IRStore(ptr, load(x))+IRCall(func, ptr, ...) to IRCall(func, x, ...) where ptr is a single use temporary previously added by updateCallSites()

Add also a minimal repro for the aforementioned case as a regression test.

Issue #9073

[skiminki-nv]

/format

[slangbot]

🌈 Formatted, please merge the changes from this PR

[csyonghe]

I don't quite understand why the problem will occur. In slang-ir-transform-params-to-constref.cpp:84, the isUseBaseAddrOperand() check will fail if the use is from the index operand of a GetElement, and therefore it will break and use the default handling logic, which is inserting a load. Why isn't the load being inserted in this case?

[csyonghe]

Adding a "fixup" phase seems like a bandaid, since the main traverseUses loop is intended to handle proper insertion of loads, and if it is not doing that we should fix it.

[skiminki-nv]

I don't quite understand why the problem will occur. In slang-ir-transform-params-to-constref.cpp:84, the isUseBaseAddrOperand() check will fail if the use is from the index operand of a GetElement ... Why isn't the load being inserted in this case?

It's not inserted when the GetElement op is already transformed because of the base operand.

the main traverseUses loop is intended to handle proper insertion of loads, and if it is not doing that we should fix it.

This would require more logic in the traverseUses loop. If the order of base/index use happened to be different (input-dependent), then I suppose we would inject the load but not transform the respective getelement.

To fix the traverseUses loop without adding the second phase, we'd probably have to take a different approach. If I'd rewrite this function, I'd use an approach that only uses semantics-preserving transformations. This would be provably correct.

1. First, inject the load to uses of the function parameter now turned to pointer. (This itself is a fixup to that the parameter was changed from value to pointer.)
2. Then, look for getelement(load(ptr), ...) and fieldextract(load(ptr), ...) patterns and transform these to load(getelementptr(ptr, ...)) and load(fieldaddr(ptr, ...)).
3. Repeat step 2 until done.

The provability comes, because the individual transformations never change the semantics of the IR.

Adding a "fixup" phase seems like a bandaid, since ...

If the word "fixup" is bothersome, I'll change it to something else. But such fixups are typical in compilers where later passes clean-up after previous passes.

[skiminki-nv]

I suppose this prevents inserting the missing load.

[skiminki-nv]

Updated the patch to avoid words like fix or fixup.

Added also another case in the test.

[csyonghe]

I guess I still don't quite understand why it fails to work if a user getElement got translated first. In that case it becomes a getElementAddr, which is not any opcode we handle in the travseseUses switch, and therefore we should insert a load before the use.

Let's not have any code that puts the IR in an invalid state if we can avoid it, so I would prefer us go with your proposed algorithm if there isn't a simple non-fixup fix to the existing code.

[skiminki-nv]

Fair enough. I'll rewrite the function.

[csyonghe]

We should check that the use is store.value, not store.destPtr before applying the change, similar to the check for "isUseBaseAddrOperand" above.

[skiminki-nv]

Right. I just reorganized this piece of existing code (from commit a6deb5e). I'll see if this can be improved a bit.

[skiminki-nv]

Actually, I'm not 100% sure I understand your suggestion.

Here we are eliminating a temporary variable that stores the parameter value, since the parameter was turned to a pointer to that value. When being turned to pointer, the ptr to the temp variable was tagged with the decoration in a previous pass, for the reason which I thought was that then we don't need to do a deeper analysis here, i.e., go through the use chain. We also know that the use is IRLoad(), since that's the worklist type.

I also did check when refactoring this code that the IR-level transformation is still done correctly for the store op, before and after my patch.

Double-checking again: (slangc -target spirv tests/language-feature/lambda/lambda-2.slang -dump-ir, dump right after calling this function)

Without this transformation (i.e., code commented out):

func %createFuncx5Fx5Fslangx5FLambdax5FcreateFuncx5F2x5Fx24x5Fx5Fsynx5Fx28x29	: Func(Float, BorrowInParam(%createFuncx5Fx5Fslangx5FLambdax5FcreateFuncx5F2_, 1 : UInt64, 2147483647 : UInt64), Float)
{
block %28(
		[nameHint("this")]
		param %this1	: BorrowInParam(%createFuncx5Fx5Fslangx5FLambdax5FcreateFuncx5F2_, 1 : UInt64, 2147483647 : UInt64),
		param %29	: Float):
	[TempCallArgImmutableVar]
	let  %30	: Ptr(%createFuncx5Fx5Fslangx5FLambdax5FcreateFuncx5F2_)	= var
	let  %31	: %createFuncx5Fx5Fslangx5FLambdax5FcreateFuncx5F2_	= load(%this1)
	store(%30, %31)
	let  %32	: Float	= call %createFuncx5Fx5Fslangx5FLambdax5FcreateFuncx5F2x5Fx28x29(%30, %29)
	return_val(%32)
}

With this transformation (i.e., code as it is in this patch):

func %createFuncx5Fx5Fslangx5FLambdax5FcreateFuncx5F2x5Fx24x5Fx5Fsynx5Fx28x29	: Func(Float, BorrowInParam(%createFuncx5Fx5Fslangx5FLambdax5FcreateFuncx5F2_, 1 : UInt64, 2147483647 : UInt64), Float)
{
block %28(
		[nameHint("this")]
		param %this1	: BorrowInParam(%createFuncx5Fx5Fslangx5FLambdax5FcreateFuncx5F2_, 1 : UInt64, 2147483647 : UInt64),
		param %29	: Float):
	let  %30	: Float	= call %createFuncx5Fx5Fslangx5FLambdax5FcreateFuncx5F2x5Fx28x29(%this1, %29)
	return_val(%30)
}

In other words, what this is doing, roughly speaking:

void func(const Type *this)
{
    const Type tmp = *this; // temporary variable to use after const Type this ==> const Type *this
    somefunction(&tmp); // this function wants this *, because its parameter type was also pointerized
}

==>

void func(const Type *this)
{
    somefunction(this);
}

Are you suggesting that the previous logic wasn't quite sound and it needs fixing and you want to make this transformation more robust? Or more generic, so that we don't depend on the decoration? Could you elaborate a bit, and also, would it be ok to do this improvement in a separate PR?

[csyonghe]

I am just saying we condition/guard the transform for OpStore only when the use is the store's val operand, not the store's dest operand, just to be safe. I would turn your assert into a condition of the if at 155. Does that make sense to you?

[csyonghe]

that is: if (as<IRStore>(userInst)->getValUse() == use && <remaining conditions>) { apply transforms }

[skiminki-nv]

Ok, so you're concerned that we might be accidentally doing this transformation based on the ptr use? I think that's not supposed to be possible, but probably doesn't hurt to be defensive here. I'll update this logic.

[csyonghe]

Suggested one change to make the logic more robust, otherwise looks good to me.