This is an archive of the discontinued LLVM Phabricator instance.

utils/update_mir_test_checks.py: allow checking fixedStack in .mir files
ClosedPublic

Authored by gbossu on Oct 10 2022, 6:43 AM.

Details

Summary

Generation of CHECK lines for fixedStack can be enabled with --print-fixed-stack.
This is particularly useful for tests which need to inspect how the
stack looks, e.g. for ABI tests.

See the other stacked revision building on top of this one which enables UTC_ARGS (in a similar fashion to other test updaters in utils/).

Diff Detail

Event Timeline

gbossu created this revision.Oct 10 2022, 6:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2022, 6:43 AM
Herald added a subscriber: arichardson. · View Herald Transcript
gbossu requested review of this revision.Oct 10 2022, 6:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2022, 6:43 AM
gbossu updated this revision to Diff 466488.Oct 10 2022, 6:49 AM

Update email address in commit...

gbossu edited the summary of this revision. (Show Details)Oct 10 2022, 7:03 AM
gbossu added reviewers: arichardson, bogner.

The overall approach seems good to me.

llvm/test/tools/UpdateTestChecks/update_mir_test_checks/Inputs/print-stack-first.mir
4–6

This comment isn't really necessary.

llvm/utils/update_mir_test_checks.py
249–250

I don't think we really have a Python coding style guide, but this should probably be func_info: FunctionInfo

271

This assignment seems pointless?

gbossu added inline comments.Nov 3 2022, 5:51 AM
llvm/test/tools/UpdateTestChecks/update_mir_test_checks/Inputs/print-stack-first.mir
4–6

I do agree, but this was brought to my attention that such a comment would be useful to people not too familiar with llvm's test structure. If you feel it's not needed or too verbose, I do not mind removing it.

llvm/utils/update_mir_test_checks.py
249–250

Will change

271

Good catch, I think it's a remnant from some conflict resolution...

nhaehnle added inline comments.Nov 3 2022, 12:35 PM
llvm/test/tools/UpdateTestChecks/update_mir_test_checks/Inputs/print-stack-first.mir
4–6

All tests in these folders are structured the same way, so it feels strange to have the comment here. But I also don't really care, it's really a bikeshed :)

gbossu updated this revision to Diff 474703.Nov 11 2022, 2:34 AM

Fix coding style & useless assignment

arsenm accepted this revision.Nov 16 2022, 4:47 PM

I don't know this code but seems reasonable

This revision is now accepted and ready to land.Nov 16 2022, 4:47 PM

Thank you! Could someone commit the change on my behalf? I do not think I have write access, since it's my first patch here.

gbossu updated this revision to Diff 478969.Nov 30 2022, 8:44 AM

Rebase on latest main, otherwise tests seem to timeout.

gbossu added a comment.Dec 1 2022, 1:07 AM

@nhaehnle @arsenm Thank you for having a look at this patch. I unfortunately don't have commit "powers", would it be possible for one of you to commit on my behalf?
Here is the commit header I usually have: Gaëtan Bossu <gaetan.bossu@amd.com>, if you need it. Thank you in advance :)

Note that there is also a stacked patch on top of this one which I would like to commit, but it's currently failing on something un-related. I'll restart the build and hope for the best I guess.

foad added a subscriber: foad.Jun 13 2023, 9:18 AM

This broke regenerating checks for tests like test/CodeGen/AMDGPU/GlobalISel/regbankselect-amdgcn.s.buffer.load.ll which use -simplify-mir and do not print the fixedStack info. Can you fix update_mir_test_checks to cope, or should we remove -simplify-mir from files with generated checks?

This broke regenerating checks for tests like test/CodeGen/AMDGPU/GlobalISel/regbankselect-amdgcn.s.buffer.load.ll which use -simplify-mir and do not print the fixedStack info. Can you fix update_mir_test_checks to cope, or should we remove -simplify-mir from files with generated checks?

Sorry about this, I'll have a look this week.

foad added a comment.Jun 14 2023, 2:04 AM

This broke regenerating checks for tests like test/CodeGen/AMDGPU/GlobalISel/regbankselect-amdgcn.s.buffer.load.ll which use -simplify-mir and do not print the fixedStack info. Can you fix update_mir_test_checks to cope, or should we remove -simplify-mir from files with generated checks?

Sorry about this, I'll have a look this week.

No worries. I actually have a simple fix already, I just need to write a test for it. I'll try to put something up for review today.

This broke regenerating checks for tests like test/CodeGen/AMDGPU/GlobalISel/regbankselect-amdgcn.s.buffer.load.ll which use -simplify-mir and do not print the fixedStack info. Can you fix update_mir_test_checks to cope, or should we remove -simplify-mir from files with generated checks?

I think -simplify-mir should just be the global default and this should tolerate anything missing

foad added a comment.Jun 14 2023, 3:50 AM

This broke regenerating checks for tests like test/CodeGen/AMDGPU/GlobalISel/regbankselect-amdgcn.s.buffer.load.ll which use -simplify-mir and do not print the fixedStack info. Can you fix update_mir_test_checks to cope, or should we remove -simplify-mir from files with generated checks?

Sorry about this, I'll have a look this week.

No worries. I actually have a simple fix already, I just need to write a test for it. I'll try to put something up for review today.

D152896