[ValueTracking] Fix "getOperand() out of range!" assertion crash

[vedantparanjape-amd]

Return instruction can also return void in LLVM IR. In such cases the number of operands are zero. So, call to getOperand(0) will result in an assertion failure as well.

Instead of using getOperand(0), we use getReturnValue() as it graciously return nullptr and prevents a crash. This bug was triggered in inliner, and this patch also adds a testcase. This patch fixes the issue #87441.

[preames]

Calling Handle with a nullptr seems surprising here. I'd add a clause to the if condition to avoid calling Handle instead.

[nikic]

Not sure this is really the right fix. If a function has a noundef attribute on the return value, then it cannot have a void return. This looks more like a context problem, in that we're dealing with two different functions during inlining.

Maybe it's an acceptable workaround, but I'm not convinced it really addresses all issues that can arise from this.

[vedantparanjape-amd]

Not sure this is really the right fix. If a function has a noundef attribute on the return value, then it cannot have a void return. This looks more like a context problem, in that we're dealing with two different functions during inlining.

I suspected this, but the LangRef for noundef isn't really clear on this aspect: https://llvm.org/docs/LangRef.html. It's not mentioned anywhere that noundef attribute on the return value means it can't have a void return.

Maybe it's an acceptable workaround, but I'm not convinced it really addresses all issues that can arise from this.

I am not sure, what more issue can this cause ?

[nikic]

Not sure this is really the right fix. If a function has a noundef attribute on the return value, then it cannot have a void return. This looks more like a context problem, in that we're dealing with two different functions during inlining.

I suspected this, but the LangRef for noundef isn't really clear on this aspect: https://llvm.org/docs/LangRef.html. It's not mentioned anywhere that noundef attribute on the return value means it can't have a void return.

Yeah, this is not spelled out in LangRef, but it's verified here:

llvm-project/llvm/lib/IR/Attributes.cpp

Lines 2067 to 2071 in 4ae33c5

	// Some attributes can apply to all "values" but there are no `void` values.
	if (Ty->isVoidTy()) {
	if (ASK & ASK_SAFE_TO_DROP)
	Incompatible.addAttribute(Attribute::NoUndef);
	}

[vedantparanjape-amd]

Not sure this is really the right fix. If a function has a noundef attribute on the return value, then it cannot have a void return. This looks more like a context problem, in that we're dealing with two different functions during inlining.

I suspected this, but the LangRef for noundef isn't really clear on this aspect: https://llvm.org/docs/LangRef.html. It's not mentioned anywhere that noundef attribute on the return value means it can't have a void return.

Yeah, this is not spelled out in LangRef, but it's verified here:

llvm-project/llvm/lib/IR/Attributes.cpp

Lines 2067 to 2071 in 4ae33c5

	// Some attributes can apply to all "values" but there are no `void` values.
	if (Ty->isVoidTy()) {
	if (ASK & ASK_SAFE_TO_DROP)
	Incompatible.addAttribute(Attribute::NoUndef);
	}

I see, this fix is just a workaround. noundef should not apply to the inlined function as such, as in atleast for ret instructions.

It is mostly happening because, somehow we are not DCE'ing ret void instructions, and before that we call simplifyinstruction

llvm-project/llvm/lib/Transforms/Utils/CloneFunction.cpp

Line 854 in 313a33b

Value *SimpleV = simplifyInstruction(I, DL);

[vedantparanjape-amd]

Not sure this is really the right fix. If a function has a noundef attribute on the return value, then it cannot have a void return. This looks more like a context problem, in that we're dealing with two different functions during inlining.

I suspected this, but the LangRef for noundef isn't really clear on this aspect: https://llvm.org/docs/LangRef.html. It's not mentioned anywhere that noundef attribute on the return value means it can't have a void return.

Yeah, this is not spelled out in LangRef, but it's verified here:

llvm-project/llvm/lib/IR/Attributes.cpp

Lines 2067 to 2071 in 4ae33c5

	// Some attributes can apply to all "values" but there are no `void` values.
	if (Ty->isVoidTy()) {
	if (ASK & ASK_SAFE_TO_DROP)
	Incompatible.addAttribute(Attribute::NoUndef);
	}

I see, this fix is just a workaround. noundef should not apply to the inlined function as such, as in atleast for ret instructions.

It is mostly happening because, somehow we are not DCE'ing ret void instructions, and before that we call simplifyinstruction

llvm-project/llvm/lib/Transforms/Utils/CloneFunction.cpp

Line 854 in 313a33b

Value *SimpleV = simplifyInstruction(I, DL);

@nikic okay I did some more analysis, so what is happening is. PruningFunctionCloner clones the basic blocks from function to be inlined into the caller function. While doing so, it handles terminator instructions like Switch/Branch, but it doesn't specially handle the Ret ones (

llvm-project/llvm/lib/Transforms/Utils/CloneFunction.cpp

Line 587 in 313a33b

// Finally, clone over the terminator.

), and clones it into the new function which has NoUndef attribute, so ideally this clone is illegal.

It becomes especially tricky as to how to handle it, given this return void statement has no successor.

[vedantparanjape-amd]

ping @nikic

[vedantparanjape-amd]

@nikic ping!

[nikic]

@vedantparanjape-amd The issue should already be fixed by #90489 -- do you still see problems?

[nikic]

Already fixed by #90489, removing this from the review queue...