Allow computed expressions on the left-hand-side #262
Conversation
|
This also needs support in polyhedral dependence analysis, putting a block for now |
|
I pushed the polyhedral support for scattering to this branch directly. Requesting @nicolasvasilache to review my code. |
|
Given that the tests already passed without the polyhedral support, should there be new tests that exercise it? |
include/tc/core/libraries.h
Outdated
| //__RETURN_TYPE isfinite ( float a ); | ||
| //__RETURN_TYPE isinf ( float a ); | ||
| //__RETURN_TYPE isnan ( float a ); | ||
| //__RETURN_TYPE isfinite ( float a ); |
There was a problem hiding this comment.
tab vs spaces war ? :)
There was a problem hiding this comment.
probably was never checked by clang-format before that
There was a problem hiding this comment.
My editor converts tabs to spaces and trims trailing whitespace on save. Feel free to revert this change if we want to preserve the tabs.
include/tc/core/libraries.h
Outdated
| @@ -32,8 +32,14 @@ namespace c { | |||
|
|
|||
| constexpr auto types = R"C( | |||
| // Halide type handling | |||
| typedef int int32; | |||
| typedef long int64; | |||
| typedef signed char int8; | |||
There was a problem hiding this comment.
Thanks for tying that loose end, deserves a separate commit?
Good call! I added polyhedral tests and by doing so found a behavior that looks incorrect. Given a simple "gather" TC It looks like range inference on the LHS does not look inside indirect accesses. Ideally, I would prefer a compilation warning here saying something like "required precondition A(i) is unique forall i will not be checked in runtime" (because that's our assumption). |
|
A(i) would have to be injective and surjective for that scatter to define a total function. We need to decide on the semantics for repeated values on A(i) and values that are never hit. It sounds like you're saying is we should permit race conditions on the writes, and we leave values that are never written as whatever is already in the buffer. If that's the case, then I think tc2halide is correct, and we just need to get sema.h to relax and not throw that error. |
Originally, TC did not support indirection on the LHS. From the polyhedral representation point of view, all writes were thus "must" writes, that is the tensor elements were necessarily overwritten. With indirection, it is impossible to decide statically which elements will be overwritten and which ones won't. Therefore, we need to separately consider "may" writes, i.e. the elements that may or may not be written depending on some dynamic values, and "must" writes. Introduce may/must write separation at the Scop level. Treat all writes as may writes, which is safe may lead to inefficient schedules.
Previous commit introduced must writes in Scop. Use them in the dependence analysis as must sources for flow dependences and as kills for flow and false dependences.
Previous commits introduced may/must writes in Scop and dependence analysis. Extract those from Halide IR. Change extractAccess to return a flag indicating whether the affine access relation is constructed is exact or not. Exact relations correspond to must writes since we statically know which tensor elements are written. Inexact relations overapproximate non-affine accesses and should be treated as may writes, assuming the tensor elements are not necessarily written.
Sema needs the list of iteration variables on the Comprehension LHS to differentiate between reduction and non-reduction variables, the former appearing only on the RHS. Original implementation assumes Comprehension LHS is a tensor whose indices are Idents and ignores more complex constructs. With indirection support, comprehensions like O(A(i)) = B(i) are possible but i is interpreted as a reduction dimension by Sema. Traverse indices of the LHS Tensor in Comprehension recursively, inspecting subtrees of Access and Apply trees and collecting all Idents.
|
Reviving this. I made Sema look inside the subscript expressions on the LHS so it no longer complains at that level. In particular, it accepts things like However, on the stupid scattering example Thoughts, @abadams @nicolasvasilache ? |
|
My inclination would be to make O(A(i)) = B(i) work if A(i) has known bounds (e.g. it's a uint8). |
|
On Wed, Apr 25, 2018 at 03:25:21PM +0000, ftynse wrote:
Reviving this. I made Sema look inside the subscript expressions on the LHS so it no longer complains at that level. In particular, it accepts things like `O(i + 2) = A(i) + B(i)` which it previously considered to be reductions, and thus failing.
However, on the stupid scattering example `O(A(i)) = B(i)`, range inference now complains about LHS index being unbounded. It indeed is, and as Andrew mentioned, i->A(i) must be a bijective relation for this to be correct. We can insert a very expensive runtime check, but I'm not sure it's a good idea so I'm inclined to just disallow that for now.
Would it be an option to simply require users to write
`O(i) = B(invA(i))` instead?
skimo
|
|
If they knew the inverse of A, they'd probably already be done. The code that inverts A is something like: invA(A(i)) = i |
And infer 0:256 as bounds for What about |
|
Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired. Before we can review or merge your code, we need you to email [email protected] with your details so we can update your status. |
No description provided.