Page MenuHomePhabricator

[Attributor] fix AAValueConstantRange and IntegerRangeState implementation
AbandonedPublic

Authored by okura on Aug 15 2020, 8:57 AM.

Details

Summary

The current deduction of AAValueConstantRange is (maybe mistakenly) too conservative.
The following are the revised points:

  1. unionKnown in IntegerRangeState ... I think we should not take union with a given range for assumed range.
  2. AAValueConstantRangeFloating::initialize
    • Although we handle CallBase case in updateImpl, we give up in initialize in the case.
    • Propagate known states for cast instructions.

Diff Detail

Event Timeline

okura created this revision.Aug 15 2020, 8:57 AM
Herald added a reviewer: homerdin. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
okura requested review of this revision.Aug 15 2020, 8:57 AM
okura added inline comments.Aug 15 2020, 9:06 AM
llvm/test/Transforms/Attributor/range.ll
2

An assumed range for the argument %depth of @rec in this test grows one by one during iterations. So we need a very big number of iterations to reach a fixpoint and I have difficulty in using attributor-max-iterations-verify option here.

Why can't we take a union of assumed range?

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
6964

I'm not sure this propagation is necessary in initialize but at least, we need TrackDpencdence = false here.

okura added inline comments.Aug 16 2020, 3:48 AM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
6964

For example, we can get [0,2) as a known range for %a instead of [0,inf) in the following case. And this propagation is needed for a value simplification in the case of @potential_test3 in potential.ll.

%a = zext i1 %bool to i32

but at least, we need TrackDpencdence = false here.

I'll fix that.

This patch addresses three unrelated things and we need to split it, assuming all points need to be addressed.

llvm/include/llvm/Transforms/IPO/Attributor.h
1956

This looks dangerous as we might end up with a known range that is not subsumed in the assumed range.

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
6964

Why wouldn't that information be propagated during the update? Could you explain where it is lost?

okura added inline comments.Aug 16 2020, 5:42 PM
llvm/include/llvm/Transforms/IPO/Attributor.h
1956

I'm not sure why a known range should be subsumed in the assumed range. Isn't it the other way around? For example, a known range and assumed range are initialized with full and empty respectively, and the assumed range is subsumed in the known range.
The following is another example. Let us assume the two returned values have the same state <full / [0,1)>. I think a clamped state for the returned position should be <full / [0,1)>. However, the clamped state is <full / full> with the current implementation.

i32 @foo(){
  ...
  ret i32 %bar
  ...
  ret i32 %baz
}
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
6964

Why wouldn't that information be propagated during the update?

I thought this propagation in initialize is sufficient but I noticed that a known range would be better during the update. I'll move this into updateImpl.

Could you explain where it is lost?

The dependency type in calculateCastInst is now REQUIRED. So If the operand's AA becomes invalid first, the known range is never propagated.

kuter added inline comments.Aug 16 2020, 5:55 PM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
6964

Adding deps in initialize is problematic. Because initialize will not be called if the dependent attribute gets changed, only the updateAA will be called. In this instance you are only using the known information, it is not as big of a deal here but we should have the 'TrackDpencdence = false` here anyways.

Aside from this
Doing this kind of propagation in initialize can be problematic in CGSCC pass since we call initialize on AAs that belong to functions other than functions that are in the SCC. I will send a patch that makes sure we don't manifest that information real soon. We should look at how this would effect the changes in the tests.

kuter added a comment.Aug 17 2020, 8:17 AM

Hi can you try this on top of D86081

okura added a comment.Aug 17 2020, 6:10 PM

Hi can you try this on top of D86081

As I mentioned above, I will move this propagation into updateImpl. Therefore, I think the problem that you are concerned about will not occur.

kuter added a comment.Aug 17 2020, 6:46 PM

Hi can you try this on top of D86081

As I mentioned above, I will move this propagation into updateImpl. Therefore, I think the problem that you are concerned about will not occur.

Sorry, I did not see the comment above.

uenoku added inline comments.Aug 19 2020, 9:35 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
1956

I think @okura is correct. I don't remember but it looks I mistakenly implemented ;)

jdoerfert added inline comments.Aug 19 2020, 2:24 PM
llvm/include/llvm/Transforms/IPO/Attributor.h
1956

I'll trust you then. Make it a standalone commit though please.

okura abandoned this revision.Aug 26 2020, 3:41 AM

This was split into some patches.