These are chat archives for halide/Halide

11th
Apr 2018
Pranav Bhandarkar
@pranavb-ca
Apr 11 15:21
@steven-johnson @zvookin - I posted one small commit after you guys approved this PR halide/Halide#2871. Ok to merge?
Andrew Adams
@abadams
Apr 11 15:22
@steven-johnson @zvookin Does anyone remember why indeterminate expressions need to propagate upwards in the simplifier? Isn't it enough for them to just exist somewhere in the expression tree?
Zalman Stern
@zvookin
Apr 11 15:23
Yea, was just waiting for the bots yesterday evening.
Pranav Bhandarkar
@pranavb-ca
Apr 11 15:24
thanks
Andrew Adams
@abadams
Apr 11 15:24
Also, @pranavb-ca while you're here, please delete any of your old defunct branches that are no longer relevant: https://github.com/halide/Halide/branches/yours
Trying to slowly erase branches that are holding onto the old github history
Pranav Bhandarkar
@pranavb-ca
Apr 11 15:24
yes, will do.
Steven Johnson
@steven-johnson
Apr 11 16:15
@abadams re: why they propagate upward, IIRC the idea is that any expr that contains indeterminate is itself indeterminate
Andrew Adams
@abadams
Apr 11 16:17
Sure, so it can be seen as constant-folding
but it seems unnecessary to do that folding
Steven Johnson
@steven-johnson
Apr 11 16:17
If it’s not necessary, eliminate it
Andrew Adams
@abadams
Apr 11 16:17
ok, I'll experiment with that
Igor Nazarenko
@inazarenko
Apr 11 21:45
looks like constraints set on buffer dimensions don't propagate. E.g. if you declare an RDom(input), the RDom dimensions will use input.extent.<D>, not input.extent.<D>.constrained. Two examples here: inazarenko/Halide@3243736 . Do you think it's worth fixing? If so, any advice as to how best to do that?
IIUC, it happens because add_image_checks injects constrained variables into the IR that exists, but not into RDom definitions that are still hiding in env at that point, nor into the other constraint expressions
Andrew Adams
@abadams
Apr 11 21:50
That sounds worth fixing, but I'm not clear on how there are still Exprs hiding in the RDoms that haven't made it into the Stmt at the time add_image_checks runs
Igor Nazarenko
@inazarenko
Apr 11 21:52
at that point, loops over RDom are defined in terms of .min and .max variables, but the LetStmts for those variables won't be created until the next pass (bounds inference)
Example:
roduce f {
let f.s0.x.loop_max = f.s0.x.max
let f.s0.x.loop_min = f.s0.x.min
let f.s0.x.loop_extent = ((f.s0.x.max + 1) - f.s0.x.min)
for (f.s0.x, f.s0.x.loop_min, f.s0.x.loop_extent) {
f(f.s0.x) = undef()
}
let f.s1.input$x.loop_extent = ((f.s1.input$x.max - f.s1.input$x.min) + 1)
let f.s1.input$x.loop_max = f.s1.input$x.max
let f.s1.input$x.loop_min = f.s1.input$x.min
let f.s1.x.loop_max = f.s1.x.max
let f.s1.x.loop_min = f.s1.x.min
let f.s1.x.loop_extent = ((f.s1.x.max + 1) - f.s1.x.min)
for (f.s1.input$x, f.s1.input$x.loop_min, f.s1.input$x.loop_extent) {
f(f.s1.input$x) = (uint8)42
}
}
Steven Johnson
@steven-johnson
Apr 11 21:56
Probably good to file an issue on it
Igor Nazarenko
@inazarenko
Apr 11 21:56
sorry, let me try that again
produce f {
  let f.s0.x.loop_max = f.s0.x.max
  let f.s0.x.loop_min = f.s0.x.min
  let f.s0.x.loop_extent = ((f.s0.x.max + 1) - f.s0.x.min)
  for (f.s0.x, f.s0.x.loop_min, f.s0.x.loop_extent) {
    f(f.s0.x) = undef()
  }
  let f.s1.input$x.loop_extent = ((f.s1.input$x.max - f.s1.input$x.min) + 1)
  let f.s1.input$x.loop_max = f.s1.input$x.max
  let f.s1.input$x.loop_min = f.s1.input$x.min
  let f.s1.x.loop_max = f.s1.x.max
  let f.s1.x.loop_min = f.s1.x.min
  let f.s1.x.loop_extent = ((f.s1.x.max + 1) - f.s1.x.min)
  for (f.s1.input$x, f.s1.input$x.loop_min, f.s1.input$x.loop_extent) {
    f(f.s1.input$x) = (uint8)42
  }
}
Andrew Adams
@abadams
Apr 11 21:57
I see. In general the thing that is supposed to happen is that passes that introduce new symbols into the Stmt after add_image_checks runs are supposed to see if there's a .constrained version of the symbol already in scope. It's not a great design, but that's the status quo.
We could try switching to a design where add_image_checks and unpack_buffers cooperate to unpack the true values using a different name, and then the constrained values are just called .min/.max
Igor Nazarenko
@inazarenko
Apr 11 21:58
One snag there is that bounds inference inserts definitions of .min and .max before the constrained variables
Andrew Adams
@abadams
Apr 11 21:58
Ah
Igor Nazarenko
@inazarenko
Apr 11 22:01
my very blurry idea of the fix was to (a) sort the definitions of .constrained variables in topological order, so that one constraint can use another; and (b) change the bounds inference to get underneath all those definitions and use them if they exist
Andrew Adams
@abadams
Apr 11 22:01
Another option is to have add_image_checks mutate the environment along with the Stmt so far
We've made a deep copy of it at that point
That might be the most general change. It wouldn't do your point (a) though
Igor Nazarenko
@inazarenko
Apr 11 22:04
yes, that sounds promising; though then we still need to change the bounds inference to insert its LetStmts after the .constrained
Igor Nazarenko
@inazarenko
Apr 11 22:20
@steven-johnson filed halide/Halide#2882