[PATCH] D140371: [Attributor] potential constant values for PHI and Load
Johannes Doerfert via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Dec 19 22:47:53 PST 2022
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
More information about the llvm-commits
mailing list