These are chat archives for halide/Halide

12th
Oct 2018
Dillon Sharlet
@dsharletg
Oct 12 00:02
that did fix it...
seems like using register_destructor is what we want here anyways
Steven Johnson
@steven-johnson
Oct 12 00:41
Looks like Travis is adding Windows support: https://blog.travis-ci.com/2018-10-11-windows-early-release
"The Windows build environment launches with support for Node.js, Rust, and Bash languages."
Welp
Janboe Ye
@JanboeYe1_twitter
Oct 12 02:51
@zvookin in should_predicate_store_load, return (bit_size == 32) && (lanes >= 4);, predicate can only handle int32. So it should be int32x16, not int16x16.
If I change vectorize(16) to vectorize(4), it works.
I do not understand why report error in min, and the type is int16x4 and int16x8
Janboe Ye
@JanboeYe1_twitter
Oct 12 03:33
@steven-johnson I reports this issue in halide/Halide#3353
Andrew Adams
@abadams
Oct 12 16:39
@dsharletg An allocation with a new_expr and a free_function releases the resource when the Free node is hit or the scope of the Allocate node ends. I don't think register_destructor does that - things persist until the calling function exits
Dillon Sharlet
@dsharletg
Oct 12 16:40
hmm, that makes sense
Andrew Adams
@abadams
Oct 12 16:40
I think it's just MemoryType::Stack that's wrong with the original code
Dillon Sharlet
@dsharletg
Oct 12 16:40
that would make a heap allocation though I think?
Andrew Adams
@abadams
Oct 12 16:40
no, new_expr means don't make an allocation - just accept a pointer
So it depends on whether or not you want a free node
If the original code wanted to release the crop at the end of the scope of the Allocation, then MemoryType::Stack is appropriate
If you want to free it at after last use, MemoryType::Heap
It's an abuse of the MemoryType enum
But those are the semantics
I feel like you may be running into preexisting bugs with crop because you're cropping in more situations than before
Dillon Sharlet
@dsharletg
Oct 12 16:43
I don't think it is, there should be no crops where they were there before
however, I do think you may have found the casue of the open bugs with async_copy_chain
Andrew Adams
@abadams
Oct 12 16:44
async_device_copy has a new crop
oh
Dillon Sharlet
@dsharletg
Oct 12 16:44
hmm
Andrew Adams
@abadams
Oct 12 16:44
no, the same crop is in master, my bad
Dillon Sharlet
@dsharletg
Oct 12 16:44
The IR does change slightly with this change
because of the extern loops
it actually seemed like things were getting simpler in a few cases... was not yet sure if those were bugs or not
Dillon Sharlet
@dsharletg
Oct 12 16:53
With Heap, this seems to have fixed the crazy GPU failures with tiled-externs
or at least two of them
need to run the rest of the tests now
Andrew Adams
@abadams
Oct 12 16:54
cool
Longer term we could expose a trigger_destructor intrinsic - that's the mechanism in CodeGen_LLVM/Posix to fire a destructor earlier than function exit
Could even try to lower allocate nodes to that
Dillon Sharlet
@dsharletg
Oct 12 16:56
couldn't this particular code be handled by just generating IR like
let result = maybe_failing()
cleanup_function()
assert(result)
then there's no need for any automatic destructing
Andrew Adams
@abadams
Oct 12 16:56
erm, good point
Marcos Slomp
@slomp
Oct 12 19:56
It seems that the GTX 750 Ti we have in the buildbots just won't cut for d3d12.
I can't reproduce any of the following errors locally on my GTX 1080
https://buildbot.halide-lang.org/master/#/builders/7/builds/57
(The GTX 750 Ti was released in early 2014, prior to d3d12 itself, which was released in the summer of 2015; nvidia provide d3d12 driver for it, but I imagine that having it running stably on such old hardware is not a priority)
Steven Johnson
@steven-johnson
Oct 12 20:22
Screen Shot 2018-10-12 at 1.22.24 PM.png
we’ve got to fix this abort() nonsense
and probably blacklist async_copy_chain for now
Dillon Sharlet
@dsharletg
Oct 12 20:25
I vaguely remembe ran option somewhere to enable that dialog
in my case, I wanted it (ages ago) so I could get that 'press retry to debug' option
in the control panel or something
Steven Johnson
@steven-johnson
Oct 12 20:26
well it’s on by default now, which we definitely don’t want
Dillon Sharlet
@dsharletg
Oct 12 20:26
but only for one of the build bots right?
Steven Johnson
@steven-johnson
Oct 12 20:26
not sure
Dillon Sharlet
@dsharletg
Oct 12 20:26
I thought one of them doesn't do this
Steven Johnson
@steven-johnson
Oct 12 20:26
just saw it on winbot1
no idea if it’s both or why it would be just one
Andrew Adams
@abadams
Oct 12 20:37
Those errors aren't async_copy_chain though - they're on random apps
Steven Johnson
@steven-johnson
Oct 12 20:37
right, but the winbot also had a half-dozen hung instances
Dillon Sharlet
@dsharletg
Oct 12 20:37
the problem might be that we are building Debug configs in VS
Andrew Adams
@abadams
Oct 12 20:37
Yeah, we could just not test the Debug config
It doesn't happen in release
But that seems like asking for trouble
Steven Johnson
@steven-johnson
Oct 12 20:38
didn’t realize we were doing debug builds, actually
Dillon Sharlet
@dsharletg
Oct 12 20:38
why asking for trouble?
Andrew Adams
@abadams
Oct 12 20:38
We distribute a debug build
Dillon Sharlet
@dsharletg
Oct 12 20:38
oh
Andrew Adams
@abadams
Oct 12 20:38
(and a release build)
Dillon Sharlet
@dsharletg
Oct 12 20:38
Well, we could figure out what option of Debug builds causes this behavior
and change just that
Steven Johnson
@steven-johnson
Oct 12 20:38
Andrew Adams
@abadams
Oct 12 20:39
Yeah, something like that
Steven Johnson
@steven-johnson
Oct 12 20:39
that’s kinda wrong, but
Andrew Adams
@abadams
Oct 12 20:39
But we only want it enabled if it's running on a buildbot. We could sniff the environment for HALIDE_BB_WORKER_NAME
Dillon Sharlet
@dsharletg
Oct 12 20:40
or just only do it for tests?
Andrew Adams
@abadams
Oct 12 20:40
The app generators are crashing.
Steven Johnson
@steven-johnson
Oct 12 20:40
disable ‘em all, worry about re-enabling it later
Dillon Sharlet
@dsharletg
Oct 12 20:40
tests -> everything but libHalide
Steven Johnson
@steven-johnson
Oct 12 20:40
Andrew Adams
@abadams
Oct 12 20:41
I think I tried that already, and it was successful for a while then stopped working
taskkill /fi "MODULES eq Halide.dll" still seems like a good idea to me as a build step
Dillon Sharlet
@dsharletg
Oct 12 20:41
Option 1 is on that link is what I was thinking
that's just some control panel option to turn this off
(or on in my case)
Steven Johnson
@steven-johnson
Oct 12 20:42
the simulate-clicking-retry approach looks simple-ish
Andrew Adams
@abadams
Oct 12 20:42
I don't think tiled_externs is going to fail anything at this point, if we want to merge it without waiting for that final builder to test the apps on llvm trunk
(if it's super-urgent)
Steven Johnson
@steven-johnson
Oct 12 20:43
Dillon Sharlet
@dsharletg
Oct 12 20:44
I was waiting for it to test GPU stuff
Andrew Adams
@abadams
Oct 12 20:44
I ran the GPU correctness tests already
just not the apps
Steven Johnson
@steven-johnson
Oct 12 20:44
(the gotcha with the hook is we’d need to patch it in for every test. if we had a test framework, that would be easy…)
Andrew Adams
@abadams
Oct 12 20:44
So it did all the async ones
Dillon Sharlet
@dsharletg
Oct 12 20:44
me too :) but build bots seemed to have slightly different behavior than local when I was testing
but yea
h
Andrew Adams
@abadams
Oct 12 20:45
s/I/it
The bot has run the GPU correctness tests
Dillon Sharlet
@dsharletg
Oct 12 20:45
related note, I found a bug fix: halide/Halide@c0f1b0a
it's on a different branch right now
I was letting the build bots finish before pushing that commit
and really it needs a test too
would you object to adding a test, running that one test manually, and then merging before the build bots finish?
Andrew Adams
@abadams
Oct 12 20:47
Wasn't this a pre-existing bug? Follow-up PR seems OK.
Unless QC is going to cut as soon as this PR is merged....
Dillon Sharlet
@dsharletg
Oct 12 20:48
eh... it was a pre-existing bug, but unlikely to ever hit it without this PR
since extern stages as outputs were unwieldly/pointless before this PR
Andrew Adams
@abadams
Oct 12 20:48
I still think a small follow-up PR is cleaner, given that the main PR is ready to merge.
Dillon Sharlet
@dsharletg
Oct 12 20:48
still another branch needed for QC so that's not relevant
OK that's no problem
Shoaib Kamil
@shoaibkamil
Oct 12 21:31
@dsharletg very cool. We can definitley leverage this (along with better GPU extern function support) to do some stuff that was much harder previously
Dillon Sharlet
@dsharletg
Oct 12 22:10
Hmm, I'm running into another case of COdeGen_C complaining about allocations not being freed, this time it is a heap allocation
is the assumption that an early free always gets injected?
that's what it looks like
if there is no Free node, this assert gets hit
I'm just going to change this codegen not to rely on destructors
Dillon Sharlet
@dsharletg
Oct 12 22:26
@abadams @zvookin do we need to retire only output crops, or input and output crops?
Alex Reinking
@alexreinking
Oct 12 22:27
Hey @dsharletg , I'm working to merge your extern loop nests into my refactor of ScheduleFunctions. Is build_loop_nest renamed from build_provide_loop_nest_helper?
Dillon Sharlet
@dsharletg
Oct 12 22:27
I believe so yes
it also changed the way it works
it takes the Stmt body as an argument instead of making the provide
and then build_provide_loop_nest makes the provide and passes it in
Alex Reinking
@alexreinking
Oct 12 22:28
Oh, that's much better, thanks
that function still has too many arguments, imo. Should just pass in the func, not all its fields
Dillon Sharlet
@dsharletg
Oct 12 22:28
If all the callers can work with that, sounds good
not sure about that though
Alex Reinking
@alexreinking
Oct 12 22:29
I'll check but IIRC it's possible
Alex Reinking
@alexreinking
Oct 12 22:41
took my first crack at merging... gonna run the tests locally
your new tests are included in test_correctness, yes?
Dillon Sharlet
@dsharletg
Oct 12 22:43
yes, though the branch affected lots of tests in test/generator as well
BTW, if you see lots of C++ compiler errors in test/generator, that might just be your local compiler
I had the same problem
didn't figure out a fix
Alex Reinking
@alexreinking
Oct 12 22:44
okay, good, so if I screwed up it should be pretty dramatic?
what compiler were you using?
Dillon Sharlet
@dsharletg
Oct 12 22:45
I would run the correctness tests, if those pass, let's let the build bots have a pass
Alex Reinking
@alexreinking
Oct 12 22:46
sounds good
Alex Reinking
@alexreinking
Oct 12 23:05
test_correctness and test_generator passed locally
Alex Reinking
@alexreinking
Oct 12 23:26
and it looks like I was able to get rid of those extra arguments
Alex Reinking
@alexreinking
Oct 12 23:41
@dsharletg just pushed would you PTAL?
Alex Reinking
@alexreinking
Oct 12 23:50
if anyone could please help me get the buildbots running on #3347, it would be much appreciated :)