This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Serialize MFInfo::ScavengeFI
ClosedPublic

Authored by sebastian-ne on Apr 27 2021, 7:43 AM.

Details

Summary

Serialize ScavengeFI from SIMachineFunctionInfo into yaml.

ScavengeFI is not used outside of the PrologEpilogInserter,
so this shouldn't change anything.

Diff Detail

Event Timeline

sebastian-ne created this revision.Apr 27 2021, 7:43 AM
sebastian-ne requested review of this revision.Apr 27 2021, 7:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2021, 7:43 AM

The point that this is only used in PEI is interesting. That almost suggests it doesn't belong here at all but I'm not sure where else you would track it.

llvm/test/CodeGen/MIR/AMDGPU/machine-function-info-no-ir.mir
352

This is rougher than I would hope. This isn't a simple integer, it's a frame index. I would hope there would be some syntax to indicate this, and some validation that it is a valid frame object. There's no frame info here at all. e.g. I would expect a string here that looks like a frame index as it appears in an operand

sebastian-ne marked an inline comment as done.

This is rougher than I would hope. This isn't a simple integer, it's a frame index. I would hope there would be some syntax to indicate this, and some validation that it is a valid frame object. There's no frame info here at all. e.g. I would expect a string here that looks like a frame index as it appears in an operand

Ah, I was already wondering why it felt so easy.
Now it uses the same form as in operands. The disadvantage is that a MachineFrameInfo is needed for serialization and deserialization.

arsenm added inline comments.Apr 30 2021, 2:54 PM
llvm/test/CodeGen/MIR/AMDGPU/invalid-frame-index2.mir
14

Can you also add a test where the stack reference does parse correctly, but there is no MachineFrameInfo?

14

Plus when there is frame info, but an out of bounds index

sebastian-ne marked 2 inline comments as done.

Add two tests to check invalid frame indices.

arsenm requested changes to this revision.May 5 2021, 2:10 PM
arsenm added inline comments.
llvm/test/CodeGen/MIR/AMDGPU/invalid-frame-index-invalid-stack.mir
5

This should emit a proper error, not just assert.

llvm/test/CodeGen/MIR/AMDGPU/invalid-frame-index-no-stack.mir
5

Ditto

This revision now requires changes to proceed.May 5 2021, 2:10 PM
sebastian-ne marked 2 inline comments as done.

Return error instead of failing assert for non-existing frame index.

I’m not quite sure how to create the SMDiagnostic. The resulting error message looks fine, so I hope this is ok.

arsenm accepted this revision.May 6 2021, 11:27 AM
This revision is now accepted and ready to land.May 6 2021, 11:27 AM
This revision was automatically updated to reflect the committed changes.