[PATCH] D33163: [Polly] Added the list of Instructions to output in ScopInfo pass

[Siddharth via Phabricator via llvm-commits]

bollu added a comment.

@nandini12396: Thanks for the patch! Other than the minor nitpicks that I have, LGTM `:)`

Nitpick: I was hoping the patch could be submitted with context. By context, I mean the  `context not available` that is written in the Phabricator UI. For a patch "with context", consider this one <https://reviews.llvm.org/rL302987>, where you have the: `▲ Show 20 Lines • Show All 57 Lines • ▼ Show 20 Line(s)` available.

Generating Context:
-------------------

Context using `git`:
--------------------

If you are uploading patches directly using `git`, the way to generate context is by using `git -U<number of lines>` or `git --unified=<number of lines>`. I don't use this, but I believe @Meinersbur mentioned using that, and that he uses `99999` or some absurdly large number like that to get all the context.

TL,DR: `$ git diff <branch> <other branch> -U999999`

Context using `arcanist`:
-------------------------

I personally use `arc`, which is a command line tool for Phabricator. It handles a lot of the stuff for you, and I find the experience much nicer. There's some documentation on how to use it for LLVM here <http://llvm.org/docs/Phabricator.html>. `arc` automatically generates the context information, so it's somewhat easier to use.

Submitting a new patch:
-----------------------

name=arc-to-new-patch
  $ arc diff --reviewers=efriedma,jdoerfert,Meinersbur,gareevroman,sebpop,zinob,huihuiz,pollydev,grosser,philip.pfaffe,etherzhhb master --nolint --nounit



- the `--reviewers` specifies who is reviewing the code. You can add / remove people from that list.
- `--nolint --nounit` asks `arc` to ignore running lint and unit tests, because Phabricator isn't setup to run these for LLVM is my understanding of this (I could be wrong, could someone confirm this?)
- `master` is the branch against which you want to generate the `diff`. this can be changed to another branch if you want to.



Updating a patch:
-----------------

name=arc-to-update
  $ arc diff --update D32639  master --nolint --nounit

- `D32639` is the patch number. You can change this to `D33163` for example for this case.
- Once again, `master` is the branch against which you want to diff.

Setting up arcanist:
--------------------

If I recall correctly, you will need to setup `git-svn` to be able to use `arcanist`. There's not much to do, except to add these lines into the `.git/config` file inside your `polly` repository:

name=.git/config
  ...
  [svn-remote "svn"]
    url = https://grosser@llvm.org/svn/llvm-project/polly/trunk
    fetch = :refs/remotes/origin/master
  ...



- There are also general instructions for `git-svn` documented here: `LINK` <http://llvm.org/docs/GettingStarted.html#developers-work-with-git-svn> should work.

- Setting up arcanist is somewhat annoying, but I believe it simplifies the experience of actually submitting a patch IMO.



================
Comment at: include/polly/ScopInfo.h:1117
+  ScopStmt(Scop &parent, BasicBlock &bb, Loop *SurroundingLoop,
+           std::vector<Instruction *> BBToInst);
 
----------------
Would it be possible to have the `std::vector<Instructor*> BBToInst` be moved? As in, can the constructor be changed to:

```lang=cpp, name=new-constructor
  ScopStmt(Scop &parent, BasicBlock &bb, Loop *SurroundingLoop,
           std::vector<Instruction *> &&BBToInst);
```


================
Comment at: include/polly/ScopInfo.h:1225
+  /// Vector for Instructions in a Basic Block.
+  std::vector<Instruction *> BBToInst;
+
----------------
Are there any uses of `BBToInst` other than to print the instructions? If not, can we change it to:

```lang=cpp
std::vector<const Instruction*> BBToInst;
```

If there are other uses where we mutate the `Instruction`, then this is perfectly fine.


================
Comment at: include/polly/ScopInfo.h:1482
   void print(raw_ostream &OS) const;
+  void printInstructions(raw_ostream &OS) const;
 
----------------
Could you please add a comment to `printInstructions` explaining what it does?


https://reviews.llvm.org/D33163