This is an archive of the discontinued LLVM Phabricator instance.

Porting the example illustrating Polly from HTML to reStructuredText
ClosedPublic

Authored by singam-sanjay on Oct 2 2016, 7:37 AM.

Details

Summary

http://polly.llvm.org/example_manual_matmul.html which illustrates individual passes of Polly, has been ported to reStructuredText and necessary changes have been made to the configuration files used by SPHINX to include the new source as a part of the documentation.

Diff Detail

Repository
rL LLVM

Event Timeline

singam-sanjay retitled this revision from to Porting the example illustrating Polly from HTML to reStructuredText .
singam-sanjay updated this object.
singam-sanjay added a reviewer: grosser.
singam-sanjay set the repository for this revision to rL LLVM.
singam-sanjay added a project: Restricted Project.
singam-sanjay added a subscriber: pollydev.
grosser edited edge metadata.Oct 25 2016, 8:49 AM

Also, could you update www/experiments/matmul/runall.sh accordingly?

docs/HowToManuallyUseTheIndividualPiecesOfPolly.rst
2 ↗(On Diff #73217)

individual (lowercase)

10 ↗(On Diff #73217)

Please use maximal 80 characters / column.

14 ↗(On Diff #73217)

ONE file

33 ↗(On Diff #73217)

ARE detected

36 ↗(On Diff #73217)

I see here trailing whitespaces in my patch.

79 ↗(On Diff #73217)

Can we drop the space between "main_" and "," (and init_array_ and ",")

87 ↗(On Diff #73217)

These links do (still) not work for me? Did you test these? Do they work for you? Or did I muss something on my side? I pulled your patch with 'arc patch D25163', run ninja docs-polly-html and opened the resulting file './tools/polly/docs/html/index.html' in the browser. The command 'find . -name scops.main.dot.png' does not show any file such file in the build directory,

singam-sanjay edited edge metadata.
singam-sanjay marked 6 inline comments as done.
  1. Limited text to 80 columns wherever possible
  2. Corrected Grammatical Errors
  3. Links to pictures that visualise scops now work

Hi,

thanks for the update. The latest patch is only relative to your previous patch. Could you instead make it relative to a Polly revision (at best trunk)?

Best,
Tobias

Updated SCoP images and LLVM-IR files

Hi,

thanks for the update. It seems my last comment is still open

The latest patch is only relative to your previous patch. Could you instead make it relative to a Polly revision (at best trunk)?

Could you possibly address this, as it prevents me from testing this?

Best,
Tobias

The latest diff file was made after rebasing the local branch to commit dbc5521ef947c4b16cb1993e11a2517f5937e028.
Here's a screenshot of git log

docs/HowToManuallyUseTheIndividualPiecesOfPolly.rst
10 ↗(On Diff #73217)

Cannot be done on command output.

36 ↗(On Diff #73217)

After console ?

79 ↗(On Diff #73217)

I've also removed the spaces between *-scopsonly and , in the next line.

But wouldn't the space make the text more readable ?

87 ↗(On Diff #73217)

The links now point to pictures from polly.llvm.org website. In the future we might have to have the links point within the repository, i.e. just "experiments/matmul/..." instead of "http://polly.llvm.org/experiments/matmul/..." .

Ported the matmul example that illustrates Polly to SPHINX.

Also includes Updated SCoP images and LLVM-IR files.

Note : The hyperlinks to SCoP images point to files on the polly.llvm.org website.

Hi Sanyam,

this now already looks very good. Two things that surprised me:

  1. I cannot apply this patch with 'arc patch D25163' but get error messages that some files are outdated. I can commit this patch without phabricator, but as we might be working together for a longer time, it would be helpful if you could figure out what is wrong. Can you try to checkout this patch with arcanist yourself?
  1. The indentation is inconsistent. In some sections the text is aligned to the section headers / in others it is indented.
grosser requested changes to this revision.Jan 14 2017, 6:31 PM
grosser edited edge metadata.

Just mark this patch as still requesting some minor changes.

This revision now requires changes to proceed.Jan 14 2017, 6:31 PM

Hello Dr Grosser,

I've made the indentation consistent.

arc patch D25163 is failing for me too, with errors stating that patch applied to a different version of a file and not its current state.


This is a bit suprising given that I was able to fast-forward my branch without conflicts.

Fortunately, the errors are not due to files that are essential to the change in documentation. The errors are with intermediate .ll files generated while applying different optimisations and .png files that visualise SCoPs. The contents of these files have changed with developments in Polly. Since these are intermediate files, would it be OK to not include updates to these files and remove them from being tracked by git ?

Thank You,
Sanjay Srivallabh

Hi Sanjay,

thanks a lot for the update. We could possibly apply this patch working around this problem, but as we will be working together for a while, I think it would be really good to understand the problem as it will otherwise consistently slow us down in the future. Could you maybe try for an hour to fix it? If you find a solution -- great. If not, we will work around it.

(Also, don't feel worried that the first patch takes very long to get integrated. The main reason is that we spend a lot of time aligning our processes. Generally, after the first couple of patches, patch review is pretty fast.)

Best,
Tobias

Hello Dr Grosser,

I wasn't able to make much headway into the cause of the problem. Do you have any suggestions ?

Thank You,
Sanjay Srivallabh

Call me Tobias.

Did you try to see why this patch submission did not work? Just post the steps that you have taken, such that others can see what was tried. I will then commit the patch manually.

Hello Tobias,

I assume you meant uploading the output of git format-patch as submitting the patch, which did work.

As for trying apply this patch, I cloned a fresh repository from http://llvm.org/git/polly.git and applied patch through "arc patch D25163", after issuing the API Token associated with my account through "arc install-certificate".

Hi Sanyam,

if you cannot fix this problem I can just commit the patch manually. For this, please upload your latest patch with fixed indentation (this was not yet uploaded).

Best,
Tobias

Hi Sanjam,

can you update the patch please, that I can commit it.

singam-sanjay edited edge metadata.
singam-sanjay retitled this revision from Porting the example illustrating Polly from HTML to reStructuredText to Porting the example illustrating Polly from HTML to reStructuredText.

Updated runall.sh to suite new command examples.

Hi Sanjay,

did you also fix the indentation that I talked about a couple before. I cannot see this in the diff.

Hello Tobias,

I've fixed the indentation.

This revision was automatically updated to reflect the committed changes.

Thank you, I committed these changes!

Thank You ! This was a wonderful learning experience.

polly/trunk/www/experiments/matmul/main___%for.cond---%for.end30.jscop.interchanged