Page MenuHomePhabricator

[UpdateTestChecks][Bug41532] Add handle of basic block names.
Needs ReviewPublic

Authored by Whitney on Jul 16 2019, 1:48 PM.

Details

Summary

Fixed Bug 41532 - update_test_checks does not actually handle block names. Also rerun reassociate-after-unroll.ll.

Diff Detail

Repository
rL LLVM

Event Timeline

Whitney created this revision.Jul 16 2019, 1:48 PM
lebedev.ri added inline comments.
llvm/test/Transforms/PhaseOrdering/reassociate-after-unroll.ll
1

Is that what the script produces?

Whitney marked an inline comment as done.Jul 16 2019, 2:10 PM
Whitney added inline comments.
llvm/test/Transforms/PhaseOrdering/reassociate-after-unroll.ll
1

Do you mean this line, the original LIT test, or the modified LIT test?
The new changes are manually modified by me.

lebedev.ri marked an inline comment as done.Jul 16 2019, 2:15 PM
lebedev.ri added inline comments.
llvm/test/Transforms/PhaseOrdering/reassociate-after-unroll.ll
1

Ah, so it's https://bugs.llvm.org/show_bug.cgi?id=41532
But still, the solution is to just rerun the script when that happens,
otherwise these changes will simply be lost when it will be rerun.

Whitney marked an inline comment as done.Jul 16 2019, 2:36 PM
Whitney added inline comments.
llvm/test/Transforms/PhaseOrdering/reassociate-after-unroll.ll
1

I will address the issue in the script at a later date and try to provide a fix for PR41532, but for now I have this test case failing unnecessarily in an out-of-tree target. So I’d like to proceed with the interim fix to the test case for the time being.

Whitney updated this revision to Diff 210471.Jul 17 2019, 6:08 PM
Whitney retitled this revision from Modified reassociate-after-unroll.ll to use regex basic block names. to [UpdateTestChecks][Bug41532] Add handle of basic block names..
Whitney edited the summary of this revision. (Show Details)
Whitney added reviewers: lebedev.ri, jdoerfert, nikic.
Whitney marked an inline comment as done.Jul 19 2019, 8:22 AM
RKSimon resigned from this revision.Jul 21 2019, 7:33 AM
RKSimon added reviewers: MaskRay, gbedwell.
MaskRay added inline comments.Jul 21 2019, 10:14 PM
llvm/utils/UpdateTestChecks/common.py
139–143

I'm concerned that the % change will create a huge amount of diff.

Can you make the updated form only apply to labels?

142

IR_LABEL_RE = re.compile(r'^()([-\w\.]+)(:)$')

Whitney marked an inline comment as done.Jul 22 2019, 7:52 AM
Whitney added inline comments.
llvm/utils/UpdateTestChecks/common.py
139–143

I am trying to figure out how to match all variables which doesn't have prefix "label".

Whitney updated this revision to Diff 211106.Jul 22 2019, 8:11 AM

I cannot think of any clean way to identify labels, there are three ways that label can be:

  1. at the start of a basic block.
  2. follow by "label".
  3. incoming blocks in phinode.

@MaskRay Do you feel strong to limit the change only to label? If so, can you give me some guidance on how to do it?

I cannot think of any clean way to identify labels, there are three ways that label can be:

  1. at the start of a basic block.
  2. follow by "label".
  3. incoming blocks in phinode.

    @MaskRay Do you feel strong to limit the change only to label? If so, can you give me some guidance on how to do it?

It depends on how other reviewers think about it... If it is suggested keeping the diff minimum is a desired property, you probably have to parse phi and label constructs..

It depends on how other reviewers think about it...

Any thoughts? I prefer to keep the script clean and simple, although the one time change to the LIT tests will not be minimal.

nikic added a comment.Jul 23 2019, 4:01 AM

I'm not sure I see the value in this change in either implementation. Changing block names seem to be a pretty niche issue (haven't ever run into this myself) and I don't think handling them in update_test_checks is worth the churn it will cause.

nikic added a comment.Jul 23 2019, 4:02 AM

Maybe this could be put behind a flag and only used in the few instances where it is needed?

@nikic Thanks for the suggestion. I personally ran into the issue that reassociate-after-unroll.ll failed due to the different in basic block name, after modifying the O2 pass pipeline. And modifying the script, was the suggested way to solve the failure. I am willing to put this change behind a flag if that's preferred, but it maybe hard for user to know when to use the flag, or even know the existence of it.

Whitney updated this revision to Diff 214427.Aug 9 2019, 12:47 PM

@nikic As suggested, I added a flag to decide if to generate basic block names as variables.

@nikic As suggested, I added a flag to decide if to generate basic block names as variables.

There is still a great amount of diff. I am afraid this cannot be accepted. Can be hold off on this until block name changing becomes less niche?

lebedev.ri resigned from this revision.Sep 1 2019, 10:25 AM