Skip to content

Conversation

RKSimon
Copy link
Collaborator

@RKSimon RKSimon commented Sep 26, 2025

Matches the behaviour in ValueTracking.cpp

Frozen ISD::LOAD nodes do become an issue when we more aggressively push freeze through a DAG - so we need to find more cases where can safely unfreeze loads (e.g. constant pool)?

…on if the LoadSDNode is known to be dereferenceable

Matches the behaviour in ValueTracking.cpp

Frozen ISD::LOAD nodes do become an issue when we more aggressively push freeze through a DAG - so we need to find more cases where can safely unfreeze loads (e.g. constant pool)?
@nunoplopes
Copy link
Member

If we are talking about the result of a load (as it seems, but I'm not an expert in SDAG), then it can be poison or undef. The patch doesn't look right.

RKSimon added a commit to RKSimon/llvm-project that referenced this pull request Sep 29, 2025
…directly freeze all operands

Avoid the ReplaceAllUsesOfValueWith approach that has caused so many problems in previous attempts at this (llvm#145939).

Minor step towards llvm#149798 - eventually we're just going to use this path for all node types, but there's a large number of regressions that need addressing first (see llvm#152107).

This exposed a couple of things that need to be addressed here:
- we need to revisit frozen nodes once we've merged all frozen/unfrozen uses of a node.
- RISCVISD::READ_VLENB is never undef/poison

Many of the current regressions are due to us not doing more to avoid freeze(load) nodes - if the load is legal, its no longer going to split. I'm not certain exactly when we can split nodes - llvm#160884 tried to catch up to ValueTracking but that might still be wrong.
@RKSimon
Copy link
Collaborator Author

RKSimon commented Sep 29, 2025

If we are talking about the result of a load (as it seems, but I'm not an expert in SDAG), then it can be poison or undef. The patch doesn't look right.

I must be missing something - why is ValueTracking's isGuaranteedNotToBeUndefOrPoison allowed to do this?

if (auto *I = dyn_cast<LoadInst>(V))
if (I->hasMetadata(LLVMContext::MD_noundef) ||
I->hasMetadata(LLVMContext::MD_dereferenceable) ||
I->hasMetadata(LLVMContext::MD_dereferenceable_or_null))
return true;

@nikic
Copy link
Contributor

nikic commented Sep 29, 2025

@RKSimon !dereferenceable refers to the return value of the load. Correct me if I'm wrong, but I believe the dereferenceable property in SDAG refers to the load operand being dereferenceable.

@nikic nikic closed this Sep 29, 2025
@nikic nikic reopened this Sep 29, 2025
@RKSimon
Copy link
Collaborator Author

RKSimon commented Sep 29, 2025

of course it is - well, that's just bobbins isn't it :)

@RKSimon RKSimon closed this Sep 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants