[AMD] enhance range analysis and buffer-op only relies on range-analysis #8372
Conversation
yangshuxin
force-pushed
the
shuxin/revamp_value_range
branch
from
c610cac to
8cf91be
Compare
|
I set change the status to "ready for review" despite failures on Nvidia platform. My change is only about AMD platform, I guess my change is not the culprit. |
yangshuxin
requested review from
antiagainst,
ptillet and
zhanglx13
as code owners
yangshuxin
changed the title
yangshuxin
requested review from
CRobeck,
Jokeren,
fywkevin and
peterbell10
as code owners
yangshuxin
force-pushed
the
shuxin/revamp_value_range
branch
from
0289d6c to
68eff64
Compare
yangshuxin
changed the title
| // step 2: Get element type and size. | ||
| // e.g. addPtrOp.getType is tensor<64x64x!tt.ptr<f16>, then elemTy is | ||
| // !tt.ptr<f16>, and dereferencing elemTy gets f16. | ||
| // TODO: Not sure if we need to keep dereferencing in a loop. |
There was a problem hiding this comment.
My experience was that yes you do need to do this.
| << ((szLimit2GB > byteOfst) ? ", out or range" | ||
| : ",in range")); | ||
|
|
||
| return byteOfst <= szLimit2GB; |
There was a problem hiding this comment.
This looks great. Thank you!
| solver->load<AMD::TritonIntegerRangeAnalysis>(assumptions); | ||
| solver->load<AMD::TritonIntegerRangeAnalysis>( | ||
| assumptions, &getAnalysis<DominanceInfo>(), | ||
| /*assumeNoArithOverflow=*/true); |
There was a problem hiding this comment.
Is this a hack or what is the equivalent when a user writes Python?
There was a problem hiding this comment.
@njriasan thank you very much for code reviewing, and your initial implementation.
For this specific change, I put a comment to the RangeAnalysis.cpp. The initial motivation can be explained by this contrived example:
a and b were used in an operation indicating it is signed int.
c = a + b
We know both a and b in [0, smax). if it will not overflow, the c will fall in [0, smax), otherwise c will be in `(smin, smax). This flag is to tell if arithmetic operation never overflow.
This "optimization" seems to be useful in proving > 0, however, pretty useless for proving < specified value. I decided to ditch this "optimization" in the first place, it might keep it for a while to see if such "optimzation" is useful in the future.
For now, this optimization is turned off. Just turned on for testing. Tuning it on for testing only see only small difference.
There was a problem hiding this comment.
I removed this part since both you and Lei are confused.
third_party/amd/lib/TritonAMDGPUTransforms/ConvertToBufferOps.cpp
Outdated
Show resolved
Hide resolved
The confusing feature is to perform value-range analysis assuming arithmetic op has nsw and nuw flags (even they are not present) e.g. pid * block_size will still fit in [0, smax] despite that the pid itself is in [0, smax]
3 participants
This change fix bugs in range-analysis, and let buffer-ops use the range-analysis result to decide if it's legal to convert memory-op to buffer-ops.
It fixes https://github.com/ROCm/triton-internal/issues/1180 and ROCm#871.
The highlight are following:
Range Analysis
tl.assume. Previously, it does not consider the control flow relationship between, saytl.assume x > 0and the location of occurrence of x.make_range(begin, end), previous vr is [begin, end], now is [begin, end-1]. Small change in concept incur huge change the regression test.Buffer-ops