This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][UpdateTestChecks] Remove the extra # when scrubbing loop comments
ClosedPublic

Authored by ZhangKang on Apr 2 2020, 7:43 PM.

Details

Summary

The patch https://reviews.llvm.org/D63957 is to avoid empty string when scrubbing loop comments.

If the line only has loop comments the patch D63957 will replace it to a #, that's correct.

But if the line has someelse not only loop comments, we will get a extra #.
For example:

.LBB40_1:                               # =>This Inner Loop Header: Depth=1

The patch D63957 will get

.LBB40_1: #

The patch is to remove the extra #.

Diff Detail

Event Timeline

ZhangKang created this revision.Apr 2 2020, 7:43 PM

The case PR35812-neg-cmpxchg.ll is modified.
For below assembly:

.LBB0_1: # %L.entry
  # =>This Inner Loop Header: Depth=1
  lharx 3, 0, 5
  cmpw 4, 3
  bne 0, .LBB0_3

The patch D63957 will conserve the space: # =>This Inner Loop Header: Depth=1 will be replaced as #,
My patch match the [ \t]*, so the space will be removed and left only a #.

jsji edited reviewers, added: Restricted Project; removed: power-llvm-team.Apr 2 2020, 8:21 PM
jsji added a project: Restricted Project.
jsji added inline comments.Apr 6 2020, 7:04 AM
llvm/utils/UpdateTestChecks/asm.py
237

Why we want to scrub loop comment TWICE?
If the extra comments # is bothering you,
can we try to add one regex to scrub it AFTER SCRUB_LOOP_COMMENT_RE?

Something like:

asm = common.SCRUB_LOOP_COMMENT_RE.sub(r'#', asm)
asm = common.SCRUB_TAILING_COMMENT_TOKEN_RE.sub(r'', asm)
ZhangKang marked 2 inline comments as done.Apr 6 2020, 6:56 PM
ZhangKang added inline comments.
llvm/utils/UpdateTestChecks/asm.py
237

Yes, removing the tailing token '#' is a better method. I will add one regex to remove the token '#'.

ZhangKang updated this revision to Diff 255599.Apr 6 2020, 11:24 PM
ZhangKang marked an inline comment as done.

Update the patch to add Regex to remove the extra #.

jsji accepted this revision as: jsji.Apr 9 2020, 7:08 PM

LGTM.

This revision is now accepted and ready to land.Apr 9 2020, 7:08 PM
This revision was automatically updated to reflect the committed changes.