[PATCH] D140371: [Attributor] potential constant values for PHI and Load

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

LG.

Can you maybe add a potential value test that checks we can fold `icmp eq %l, 15` if we know `%l = load ...` is either `14` or `16`. Just to get "more direct" coverage.



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:1407
+    // Otherwise, we end up pessimizing AAPointerInfo by respecting offsets that
+    // don't actually exist.
+    //
----------------
You can add that no values implies it's assumed dead (for now).


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9484
         !fillSetWithConstantValues(A, IRPosition::value(*RHS), RHSAAPVS,
-                                   RHSContainsUndef))
+                                   RHSContainsUndef, /* ForSelf = */ false))
       return indicatePessimisticFixpoint();
----------------
Nit: For consistency with the rest of the file we should probably omit the "=" in the comments.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9522-9524
+    if (ContainsUndef)
+      unionAssumedWithUndef();
+    else {
----------------
Nit: Add the braces if the alternative has them.


================
Comment at: llvm/test/Transforms/Attributor/value-simplify.ll:191
 ; CHECK-NEXT:    [[PHI_SAME_UNDEF:%.*]] = phi i32 [ 1, [[IF_TRUE]] ], [ undef, [[IF_FALSE]] ]
+; CHECK-NEXT:    [[SELECT_NOT_SAME_UNDEF:%.*]] = select i1 [[C]], i32 [[PHI_NOT_SAME]], i32 undef
 ; CHECK-NEXT:    tail call void @use(i32 noundef 1)
----------------
sameerds wrote:
> This optimization was a fluke. Now that the constant vaules of `%phi-not-same` are exposed, `%select-not-same-undef` is no longer simplified to a single value. This needs further simplification using constant values in `getAssumedSimplified()`, similar to how AAPotentialConstantValues looks at the constant values of each operand.
I wouldn't call it fluke but it's unclear what the right result here is. I recently made AAPotentialValues keep selects and phis if it couldn't optimize them into a single value, which is what most users want. However, I can see that for constants a list of values might be more valuable (pun intended). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140371