Skip to content

[GlobalISel] Document minimum legality requirements for G_IMPLICIT_DEF. #117609

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 10, 2024
Merged

[GlobalISel] Document minimum legality requirements for G_IMPLICIT_DEF. #117609

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f6d47a8
[GlobalISel] Document minimum legality requirements for G_IMPLICIT_DEF.
aemerson Nov 25, 2024
abea77e
Remove imprecise statement.
aemerson Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions llvm/docs/GlobalISel/Legalizer.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,8 +338,8 @@ G_BUILD_VECTOR_TRUNC, G_CONCAT_VECTORS, G_UNMERGE_VALUES, G_PTRTOINT, and
G_INTTOPTR have already been noted above. In addition to those, the following
operations have requirements:

* At least one G_IMPLICIT_DEF must be legal. This is usually trivial as it
requires no code to be selected.
* For every type that can be produced by any instruction, G_IMPLICIT_DEF must be
legal. This is usually trivial as it requires no code to be selected.
Copy link

@tschuett tschuett Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could create illegal MIR after the legalizer. Please tell me when you finished inspecting all downstream legalizers.

Copy link
Contributor Author

@aemerson aemerson Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could create illegal MIR after the legalizer. Please tell me when you finished inspecting all downstream legalizers.

So two things here:

  1. We are requiring that targets make something legal. It's down to the targets to ensure this happens, not generic GlobalISel code.
  2. Even if the first point didn't exist, which particular downstream legalizers do you want me to look at?

Copy link

@tschuett tschuett Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AArch64 legalizer disagrees/is nonconforming.

I see the legalizer as the ground-truth for legality questions. I prefer to query the legalizer and not enforce rules on the legalizer.

I don't see any benefit of this change. It makes the state worth. It gives you an escape-hatch that you don't have to query the legalizer in some situations.

All non-Apple:-)

Copy link
Contributor

@arsenm arsenm Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as it requires no code to be selected. is not really true. G_IMPLICIT_DEF should select to IMPLICIT_DEF.

My concern was reliance on introduction of new G_IMPLICIT_DEFs in the artifact combiner, such that not having a legal option could fail. I'm not actually seeing where that might happen.

However targets are doing a disservice to themselves by not having G_IMPLICIT_DEF as legal for every possible register type. I know SelectionDAG has an anti-feature that I've run into before where the default expansion of UNDEF turns into a zero

Copy link

@tschuett tschuett Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

llvm-project/llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp

Line 95 in bb88fd1

{G_IMPLICIT_DEF, G_FREEZE, G_CONSTANT_FOLD_BARRIER})

G_IMPLICIT_DEF is not legal for scalable vectors!

Copy link
Contributor

@arsenm arsenm Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which should be considered a target bug. In 100% of situations this should select to IMPLICIT_DEF

Copy link

@tschuett tschuett Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am more worried about illegal MIR than how hard it is to select G_IMPLICIT_DEF.

Copy link
Contributor Author

@aemerson aemerson Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AArch64 legalizer disagrees/is nonconforming.

I see the legalizer as the ground-truth for legality questions. I prefer to query the legalizer and not enforce rules on the legalizer.

You're welcome to see things how you like. That has no relation to reality however.

llvm-project/llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp

Line 95 in bb88fd1

{G_IMPLICIT_DEF, G_FREEZE, G_CONSTANT_FOLD_BARRIER})

G_IMPLICIT_DEF is not legal for scalable vectors!

That sounds like something we should fix at some point. Were you under the impression that scalable vector support was complete?

I am more worried about illegal MIR than how hard it is to select G_IMPLICIT_DEF.

We are clarifying the definition what is assumed to be legal, and therefore it is not illegal MIR. Any complaints about illegal MIR are therefore bugs or missing support in the target implementation.

Are we clear now?

* G_PHI must be legal for all types in the producer and consumer typesets. This
is usually trivial as it requires no code to be selected.
* At least one G_FRAME_INDEX must be legal
Expand Down
Loading