[IR] Allow type change in ValueAsMetadata::handleRAUW

[jasilvanus]

ValueAsMetadata::handleRAUW is a mechanism to replace all metadata referring to one value by a different value.

Relax an assert that used to enforce the old and new value to have the same type.
This seems to be a sanity plausibility assert only, as the implementation actually supports mismatching types.

This is motivated by a downstream mechanism where we use poison ValueAsMetadata values to annotate pointee types of opaque pointer function arguments.

When replacing one type with a different one to work around DXIL vs LLVM incompatibilities, we need to update type annotations, and handleRAUW is more efficient than creating new MD nodes.

[llvmbot]

@llvm/pr-subscribers-llvm-ir

Author: Jannik Silvanus (jasilvanus)

Changes

ValueAsMetadata::handleRAUW is a mechanism to replace all metadata referring to one value by a different value.

Relax an assert that used to enforce the old and new value to have the same type.
This seems to be a sanity plausibility assert only, as the implementation actually supports mismatching types.

This is motivated by a downstream mechanism where we use poison ValueAsMetadata values to annotate pointee types of opaque pointer function arguments.

When replacing one type with a different one to work around DXIL vs LLVM incompatibilities, we need to update type annotations, and handleRAUW is more efficient than creating new MD nodes.

---

Full diff: https://github.com/llvm/llvm-project/pull/76969.diff

1 Files Affected:

• (modified) llvm/lib/IR/Metadata.cpp (+1-1)

diff --git a/llvm/lib/IR/Metadata.cpp b/llvm/lib/IR/Metadata.cpp
index 515893d079b8cb..bdfbd8829186d9 100644
--- a/llvm/lib/IR/Metadata.cpp
+++ b/llvm/lib/IR/Metadata.cpp
@@ -503,7 +503,7 @@ void ValueAsMetadata::handleRAUW(Value *From, Value *To) {
   assert(From && "Expected valid value");
   assert(To && "Expected valid value");
   assert(From != To && "Expected changed value");
-  assert(From->getType() == To->getType() && "Unexpected type change");
+  assert(&From->getContext() == &To->getContext() && "Expected same context");
 
   LLVMContext &Context = From->getType()->getContext();
   auto &Store = Context.pImpl->ValuesAsMetadata;

[jmorse]

I feel like this is well motivated, and as you say there aren't actually any parts of LLVM that rely on ValueAsMetadata types for correctness (AFAIUI). Would I be right in thinking that this only enables use-cases that are directly calling ValueAsMetadata::handleRAUW, because performing a normal RAUW will do the usual type checks?

CC @OCHyams and @SLTozer as they've touched this area more recently than me.

[OCHyams]

This seems fairly harmless to me, especially as @jmorse points out Value::replaceAllUsesWith will still check types are the same through Value::doRAUW, though I'm not sure I understand the use-case very well.

Would it be possible for you to add unittest showing this in action? Otherwise SGTM.

[SLTozer]

Agreed with this being harmless - not all cases of ValueAsMetadata::handleRAUW pass through Value::doRAUW, and by the sounds of it this isn't happening in a typical RAUW case? I don't think there's any context where changing the type of a VAM in a debug info context would cause an error; though if your case only involves poison types, it might be safer to change the assert to:
assert((isa<PoisonValue>(To->getType()) || From->getType() == To->getType()) && "Expected same or poison type");
This wouldn't weaken the existing guarantees very much, while enabling your use case if I understand correctly - if it's necessary or more convenient to use the existing approach though then I have no objections, though as above adding a motivating test case would be good.

[jasilvanus]

Would it be possible for you to add unittest showing this in action? Otherwise SGTM.

Will do.

though if your case only involves poison types, it might be safer to change the assert to: assert((isa<PoisonValue>(To->getType()) || From->getType() == To->getType()) && "Expected same or poison type"); This wouldn't weaken the existing guarantees very much, while enabling your use case if I understand correctly

I considered this, but I think such a constraint would be a bit arbitrary. What about undef? Zero? There doesn't seem to be a fundamental reason to forbid such type changes.

• if it's necessary or more convenient to use the existing approach though then I have no objections, though as above adding a motivating test case would be good.

I'll add a test case, but it'll only exercise usage rather then giving a true motivation.

To put our application outlined in the PR description in other words, we are using poison ValueAsMetadata values to essentially build a fake TypeAsMetadata, and changing such type metadata requires to change the type of the used underlying ValueAsMetadata nodes.

[jmorse]

I'll add a test case, but it'll only exercise usage rather then giving a true motivation.

IMO this is fine, it discourages people from breaking behaviours that you're relying on. LGTM.

[SLTozer]

I considered this, but I think such a constraint would be a bit arbitrary. What about undef? Zero? There doesn't seem to be a fundamental reason to forbid such type changes.

I think your current approach is fine so no need to adjust but FWIW, though it may be arbitrary, UndefValue and PoisonValue by extension do have special meaning in metadata contexts since it's what we use to represent dead debug values; by the sounds of it it's also being given a special meaning in your downstream context, so - noting this for posterity's sake only, in case this comes up again - I think there's a fair argument to start giving it special treatment, or else to actually define a new Value-type that is expected to only appear in metadata contexts to be used for both these cases (which would prevent unrelated changes in IR semantics/style from requiring debug info rewrites, as with the UndefValue->PoisonValue updates).