[PATCH] D142687: [Local] Don't keep K's range even if K dominates J

[Nikita Popov via Phabricator via llvm-commits]

nikic requested changes to this revision.
nikic added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/test/Transforms/GVN/range.ll:109
+define i32 @load_noundef_load(ptr %p) {
+; CHECK-LABEL: define i32 @load_noundef_load
+; CHECK-SAME: (ptr [[P:%.*]]) {
----------------
StephenFan wrote:
> 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?
> 
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.


================
Comment at: llvm/test/Transforms/JumpThreading/thread-loads.ll:334
 ;
   %a = load i32, ptr %0, !tbaa !0, !range !4, !alias.scope !9, !noalias !10
   %b = load i32, ptr %0, !range !5
----------------
Probably better to add `!noundef` here, otherwise this doesn't really match the code comment anymore.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142687/new/

https://reviews.llvm.org/D142687