[Local] Use most generic range if K does not dominate J or K doesn't have a !noundef

[StephenFan]

Summary

Since D141386 has changed the return value of !range from IUB to poison,
metadata !range shouldn't be preserved even if K dominates J.

If this patch was accepted, I plan to adjust metadata !nonnull as well.
BTW, I found that metadata !noundef is not handled in combineMetadata,
is this intentional?

[nikic]

BTW, I found that metadata !noundef is not handled in combineMetadata, is this intentional?

No. I think we should actually fix this first, and then retain the !range of the first load if !noundef is also set. This will make sure that there are no performance regressions from this change.

[nikic]

The !noundef preservation fixes have landed, so I think this patch can now be adjusted to keep the metadata if !noundef is present on the dominating load.

[StephenFan]

Keep the metadata if J has !noundef

[nikic]

Could you please also add tests for the !noundef case? (Preferably one with the first load noundef and one with the second. I'm not sure you're checking the right one currently.)

[StephenFan]

Rebase and add test for load with !noundef

[StephenFan]

[StephenFan]

llvm/test/Transforms/GVN/range.ll

109

Now, this case is combined into the most generic range. But I think it is unnecessary.
Before transforming, there are 4 cases:

1. Both are satisfied. Return a well-defined value.
2. Both are violated. IUB.
3. !0 is violated and !1 is satisfied. IUB.
4. !0 is satisfied and !1 is violated. Return a poison value.

If we don't combine them into the most generic range and just retain !0.

1. Both are satisfied. Return a well-defined value.
2. Both are violated. IUB.
3. !0 is violated and !1 is satisfied. IUB.
4. !0 is satisfied and !1 is violated. Return a well-defined value.

The only difference is case 4. We transform returning a poison value to returning a well-defined value. But it is safe.

In summary, I think we don't need to combine them into the most generic range for this case, just retaining it is enough.
Do you agree with me @nikic?

[nikic]

llvm/test/Transforms/GVN/range.ll

109

Right, this is why I said you're checking noundef on the wrong instruction. It needs to be on K, not J. You can see in the case below that range !0 is retained, even though that one should actually use the generic range.

llvm/test/Transforms/JumpThreading/thread-loads.ll

330

Probably better to add !noundef here, otherwise this doesn't really match the code comment anymore.

[StephenFan]

llvm/test/Transforms/GVN/range.ll

121

@nikic Why do we need to use the generic range here?

1. Both are satisfied. Return a well-defined value.
2. Both are violated, IUB
3. !0 is satisified, !1 is violated. IUB.
4. !0 is violated, !1 is satisified. return a poison value.

If we retain !0:

1. Both are satisfied. Return a well-defined value.
2. Both are violated. Return a poison value.
3. !0 is satisfied, !1 is violated. Return a well-defined value
4. !0 is violated, !1 is satisfied. Return a poison value.

The differences are in case 2 and case 3. In case 2, transforming from IUB to return a poison value is safe. In case 3, transforming from IUB to return a poison value is also safe. Or do I misunderstand something?

[nikic]

llvm/test/Transforms/GVN/range.ll

121

You need to assume that the example is simplified, and what actually happens is that %a is never used (dynamically dead). In that case if !0 is violated but !1 is satisfied, this would return a poison value for %b, which is incorrect.

It's okay in this specific code sample (where both values are fed into the same operation), but this is not the point of these tests.

[StephenFan]

Use most generic range if K don't have a !noundef

[StephenFan]

Update title.

[nikic]

LGTM