Change list striding parameters to take optional integer (preceding work for #49352)

[tugsbayasgalan]

Stack from ghstack:

• Striding for lists Part 2 #49352 Striding for lists Part 2
• Change list striding parameters to take optional integer (preceding work for #49352) #48719 Striding for lists Part 1

Summary: Attempt to break this PR (#33019) into two parts. As per our discussion with @eellison, the first part is to make sure our aten::slice operator take optional parameters for begin/step/end. This will help with refactoring ir_emitter.cpp for genering handling for list and slice striding. Once this PR merged, we will submit a second PR with compiler change.

Test Plan: None for this PR, but new tests will be added for the second part.

Reviewers:

Subscribers:

Tasks:

Tags:

Differential Revision: D25929902

[dr-ci]

💊 CI failures summary and remediations

As of commit 23aaf43 (more details on the Dr. CI page):

---

• 3/3 failures possibly* introduced in this PR
  • 2/3 non-CircleCI failure(s)

---

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

pytorch_xla_linux_bionic_py3_6_clang9_build (1/1)

Step: "Build" (full log | diagnosis details | 🔁 rerun)

Jan 12 01:57:23 Failed to generate ATEN bindings: ['/var/lib/jenkins/workspace/xla/scripts/generate_code.sh']

Jan 12 01:57:22 AtenXlaType function missed override: Tensor slice(const Tensor& self, int64_t dim, int64_t start, int64_t end, int64_t step); // slice(Tensor,int64_t,int64_t,int64_t,int64_t)->Tensor
Jan 12 01:57:22 Traceback (most recent call last):
Jan 12 01:57:22   File "/var/lib/jenkins/workspace/xla/scripts/gen.py", line 1195, in <module>
Jan 12 01:57:22     generate(args)
Jan 12 01:57:22   File "/var/lib/jenkins/workspace/xla/scripts/gen.py", line 1165, in generate
Jan 12 01:57:22     assert check_overrides(overrides, overridden)
Jan 12 01:57:22 AssertionError
Jan 12 01:57:23 Building torch_xla version: 1.6
Jan 12 01:57:23 XLA Commit ID: e62799edcc8ef199e3903ef778b6715aa8a16468
Jan 12 01:57:23 PyTorch Commit ID: 23aaf43192dc73da6a1b6c91648316f227d33e8f
Jan 12 01:57:23 Failed to generate ATEN bindings: ['/var/lib/jenkins/workspace/xla/scripts/generate_code.sh']
Jan 12 01:57:23 =================== sccache compilation log ===================
Jan 12 01:57:23 + cleanup
Jan 12 01:57:23 + retcode=1
Jan 12 01:57:23 + set +x
Jan 12 01:57:23 =========== If your build fails, please take a look at the log above for possible reasons ===========
Jan 12 01:57:23 Compile requests                    4738
Jan 12 01:57:23 Compile requests executed           4442
Jan 12 01:57:23 Cache hits                          3143
Jan 12 01:57:23 Cache hits (C/C++)                  3143
Jan 12 01:57:23 Cache misses                        1281

---

Extra GitHub checks: 2 failed

• Failed: GitHub Actions - quick-checks
• Failed: GitHub Actions - clang-format

---

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

This comment has been revised 98 times.

[albanD]

What is actually changing here? It would be nice to have the codemod in a separate PR.

[eellison]

Yea, agreed with alban, it might be easier to review this once the whole stack is out

[eellison]

cc @ailzhang XLA failure

[eellison]

Some context: we are fixing the lists slicing logic with a negative index. Part of this change involves changing the ir emission for aten::slice to emit None for non-present step value instead of INT_MAX. Rather than have special case logic for all of the builtins type that may or may not support negative striding (lists, tensors, strs) we are updating their implementations so that they can all take in optionals. Actual negative slicing of Tensors is still NYI, but making the schema changes here makes future implementation easier and makes the logic in ir emission cleaner.

[eellison]

Looks great! Nice job wading through the tracer code.

I have two comments:

• I don't think we actually need to make step optional, I think python just defaults it to 1
• I'm not sure about the TenssorShape.cpp changes, I think it would be better to error at negative step within TensorShape instead of trying to implement it for this PR.

[eellison]

LGTM, Ship it! Great job! We might want to sanity check the same forward-compatibility checks we did previously with the newest changes, just to be safe. But up to you.

[eellison]

LGTM, ship it! We should wait a couple weeks or so before landing PT2 so we dont run into FC issues.

[facebook-github-bot]

This pull request has been merged in 1a38fa9.

[ezyang]

Why does this default to INT64_MAX? Why not just set it to 0? cc @eellison this is impeding SymInt-ification

[eellison]

Ugh, I forget, but I think that was mirroring how the python slice handling code went where None would be encoded as INT64_MAX

[eellison]

It may have also been a forward compatible thing because None support had to land in multiple parts