This is an archive of the discontinued LLVM Phabricator instance.

Improve running time of getAsmSrcLocInfo
ClosedPublic

Authored by yunlian on Nov 6 2015, 11:35 PM.

Details

Summary

This tries to improve the compilation time of the test case in
https://llvm.org/bugs/show_bug.cgi?id=25416

The problem is that when trying to get the SrcLocInfo for asm, it tries to get the SrcLoc for each
line inside the asm string. For each line, it goes through the token table once. This is not optimal,
we can find the SrcLoc for all the lines of the asm with a single scan of the token table. To do this,
we need to store the information of the location of current token and the total size of token been
processed.

Diff Detail

Event Timeline

yunlian updated this revision to Diff 39634.Nov 6 2015, 11:35 PM
yunlian retitled this revision from to Improve running time of getAsmSrcLocInfo.
yunlian updated this object.
yunlian added a reviewer: echristo.
yunlian set the repository for this revision to rL LLVM.
yunlian added a project: Restricted Project.
yunlian updated this revision to Diff 39637.Nov 6 2015, 11:53 PM
yunlian removed rL LLVM as the repository for this revision.

Some format changes.

rafael edited edge metadata.
rafael added a subscriber: llvm-commits.

Can you add a testcase? Anything that used to take 5s and is now much faster should get noticed if this regresses.

Is there an example for the test case with timeout? Thanks

rsmith added inline comments.Nov 10 2015, 6:17 PM
include/clang/AST/Expr.h
1613–1618

Please run this through clang-format.

lib/AST/Expr.cpp
1009

Please update the doc comment to explain what the new parameters do and how they should be used.

1013

StartTokenByteOffset or ByteOffsetOfStartToken would be more obvious names here.

yunlian updated this revision to Diff 39945.Nov 11 2015, 10:51 AM
yunlian edited edge metadata.

The clang-format check is done. The doc comment is updated.

yunlian marked 3 inline comments as done.Nov 16 2015, 9:03 AM

ping?

rafael added inline comments.Nov 16 2015, 1:13 PM
include/clang/AST/Expr.h
1615

Are you sure you used clang-format? The pointer style we use in llvm is "type *var".

One way to do it is to run "git clang-format master", assuming you have this patch in a git branch.

yunlian updated this revision to Diff 40349.Nov 16 2015, 2:55 PM

I did not realize that git clang-format will modify the code directly. Updated.
Thanks

rafael added inline comments.Nov 16 2015, 4:15 PM
lib/AST/Expr.cpp
1030

This part also needs git-clang-format..

yunlian added inline comments.Nov 16 2015, 4:26 PM
lib/AST/Expr.cpp
1030

Which part? Could you please give me some advice on how to modify it?
Thanks
I run git clang-format and it says clang-format did not modify any files.

if (StartToken) TokNo = *StartToken;

It should have a newline.

yunlian added inline comments.Nov 16 2015, 4:34 PM
lib/AST/Expr.cpp
1030

For this line:
if (StartToken) TokNo = *StartToken;

I try to make it like
if (StartToken)

TokNo = *StartToken;

Bug git clang-format changes it back.

There is something strange with your git-clang-format setup. This is
what I get locally.

Cheers,
Rafael

yunlian updated this revision to Diff 40401.Nov 17 2015, 9:01 AM

Applied Rafael's patch while still looking to see what is wrong of my git clang format.

David, did you also have a clang-format issue where it wasn't breaking
short if's? In the end how did you fix it? Yunlian seems to be having a
similar problem.

  • Sean Silva
emaste added a subscriber: emaste.Nov 23 2015, 10:01 AM
rsmith accepted this revision.Dec 3 2015, 4:42 PM
rsmith edited edge metadata.

LGTM

A testcase would be nice, but I guess the speedup here isn't dramatic enough to really be testable with an test case of a sane size (quadratic to linear in the size of the input)?

This revision is now accepted and ready to land.Dec 3 2015, 4:42 PM

A test case with 4K lines of code can show the difference. Reducing the compilation time from 3 seconds to instant on my desktop. Do you need that?
Could you please commit this for me? I still do not have the right to commit yet.

rsmith closed this revision.Dec 9 2015, 5:14 PM

Committed as r255198.