Use an explicit value mapping instead of IRReplaceSizes()

[tlemo]

The core of this change is in lower_validation.cpp & fusion.cpp : instead of mutating the IR during PrepareForLowering(), we build a Val* -> NamedScalar* map, which is part of the Fusion object and used during kernel code generation.

This simplifies the lowering logic, avoids a significant number of IR node allocations and importantly it allows a simple model for expression evaluation: things work the same before and after lowering (so this is fixing #87)

[csarofeen]

There's a lot of stylistic changes in the PR, some fine, some I think are a bit questionable. Stylistic changes are fine to adhere to convention, but not at decreased readability. You'll see my potentially obnoxious comments below asking for clarity on some changes, while others that I didn't comment seem fine.

The general approach seems reasonable to unblock other work, however, I'm curious what other options you considered before going with a val map. I'm concerned about general usage of it moving forward. Are we still in agreement that we would like a hard separation between the user IR and a lowered IR with its own container? If so, I think it would be nice to put some TODO's or comments that we will be unlikely to continue with the value map approach if so. I'm particularly concerned about potential confusion when using this map/working with it as if someone arbitrarily uses it to store some information, you have to explicitly look for that information. I'm thinking someone may store information in here in a lowering pass, and then someone working on a later pass wouldn't know to look for that information in this map.

[csarofeen]

Please leave this check for now as it's not checked inside the merge/split functions. If we did not compute an index it is a hard error, and helps prevent me from shooting myself in the foot. If we get to a point where the checks cause high latency, or we have a bullet proof codegen then we can get rid of it.

[tlemo]

The check is not gone (index_map_.at(id) is checked access). We do lose the developer friendly message, if you think that's important here I'll revert this change.

[csarofeen]

I always forget about at. That makes sense, can you put the error message in a comment before the at call so we have a breadcrumb for ourselves.

[tlemo]

done

[csarofeen]

Would it make sense in our printers to add an output/warning if nothing will be printed like "No value map entries found."

[tlemo]

Do you mean, make it obvious that there's nothing to print? The code below does that by bracing the values between the header/footer lines. Empty map would look like:

Values map
-------------------------
-------------------------

[csarofeen]

I was thinking a small message like "Warning: No values in map" between the lines would be a bit more intuitive.

[csarofeen]

Will we have to add this to every all other types?

[tlemo]

Right now we only map Ints (sizes), so this would be the most likely point of failure. I didn't want to spray this paranoid check everywhere, although I could be easily be convinced to do so.

[csarofeen]

Given that this is a stopgap, yes, that makes sense.

[csarofeen]

Why bother when we're guaranteed to set it to something below?
If it's triggering, why not:

Val* size = alloc_dims.size() == 0 ? new Int(1) : alloc_dims[0];

[tlemo]

1. Consistency. It builds muscle memory, for best effect this should be a pattern where one doesn't have to think about twice.

2. The practice guards no only against current code, but also future modifications.

[csarofeen]

why?

[tlemo]

Why was it needed in the first place?

This is the only place we do it, so it's inconsistent. Besides, are you confident that future modifications would correctly duplicate the conditions in the comments?

[csarofeen]

Why?

[tlemo]

reverted

[csarofeen]

why?

[tlemo]

just a minor simplification (I think I rewrote more of this code, then reverted most of it)

If you don't like it I'll revert this change.

[csarofeen]

On second review the auto on the for loop argument is fine as the name pretty strictly implies it's a TensorView. The auto on the root_td I dislike, as the naming is conflicting with types. getRootDomain explicitly does not return a TensorDomain and it's important to keep in mind that this type is a vector of IterDomains, not a TensorDomain.

Generally I do not prefer auto over stricter typing and myself should use auto more sparingly. Many people still don't use modern IDEs and figuring out types can be painful.

[tlemo]

reverted

[tlemo]

The auto on the root_td I dislike, as the naming is conflicting with types. getRootDomain explicitly does not return a TensorDomain and it's important to keep in mind that this type is a vector of IterDomains, not a TensorDomain.

This is a good point. Would it make sense to rename getRootDomain() (and TendorDomain::rootDomain())? Or make them return TensorDomain?

[csarofeen]

Why?

[tlemo]

The main style that we seem to follow prescribes that abbreviations and acronyms to be spelled as regular words (StartRpc() rather than StartRPC()) : https://google.github.io/styleguide/cppguide.html#General_Naming_Rules

I'm fine with deviating from this style if we document it and do it consistently (although my recommendation is to stick to the well documented Google style - if anyone has any stylistic questions we can point them there)

[tlemo]

Re. the general direction, I agree, there are many things I don't like about the mapping approach, but I think it's good as a stop gap. (the fact that the IR is not heavily mutated during lowering is an important step towards a clear lowering boundary)

I'd love to hear any suggestions on how we can make sure this mapping is not getting abused in the meantime.

[csarofeen]

How does something like this seem to you?
https://github.com/csarofeen/pytorch/blob/manual/test/cpp/jit/test_gpu.cpp#L3264-L3359

[tlemo]

It could work. Looks a bit too fuzzy to me ("foo bar" would compare equal to "foobar"), so it would be nice if it collapses spaces between identifiers (to a single space) instead of removing them completely, but in this case it may be good enough.

[csarofeen]

Looks good, feel free to merge.