[GlobalISel] Document minimum legality requirements for G_IMPLICIT_DEF.

[aemerson]

So that @tschuett can fix up #117439

The reason for this change is to clarify an existing technical restriction of LLVM: there needs to be a way to implicitly define a type if there is any way to legally define that type by another means.

[llvmbot]

@llvm/pr-subscribers-llvm-globalisel

Author: Amara Emerson (aemerson)

Changes

So that @tschuett can fix up #117439

---

Full diff: https://github.com/llvm/llvm-project/pull/117609.diff

1 Files Affected:

• (modified) llvm/docs/GlobalISel/Legalizer.rst (+2-2)

diff --git a/llvm/docs/GlobalISel/Legalizer.rst b/llvm/docs/GlobalISel/Legalizer.rst
index 1ff7b304b3a013..cd27a0249c4b3e 100644
--- a/llvm/docs/GlobalISel/Legalizer.rst
+++ b/llvm/docs/GlobalISel/Legalizer.rst
@@ -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.
 * 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

[tschuett]

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

[aemerson]

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?

[tschuett]

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:-)

[arsenm]

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

[tschuett]

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!

[arsenm]

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

[tschuett]

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

[aemerson]

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?

[aemerson]

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.

That's the entire point of this change. It's pointless to have to query for legality and slow down compilation when LLVM codegen fundamentally breaks when some constructs are not legal.

All non-Apple:-)

Ok, I was asking to give you the benefit of the doubt. Having clarified that you meant check downstream targets that are private to myself, you're now admitting that your statement before was entirely sarcastic without any purpose in the discussion?

[tschuett]

You can just an add cache to the legalizer instead of introducing harsher rules. As a fall-back IMPLICIT_DEF is still available.

[tschuett]

The summary should include a motivation for that change.

[aemerson]

You can just an add cache to the legalizer instead of introducing harsher rules. As a fall-back IMPLICIT_DEF is still available.

Caches are not free either. I'll change the wording in this PR but this is documenting existing de facto convention for good reason. Your feedback has been noted.

[tschuett]

I disagree with that change. It put's unnecessary burden on implementations with limited gain.

[aemerson]

I disagree with that change. It put's unnecessary burden on implementations with limited gain.

What burden are you talking about? How does this change anything that isn't already the case? I'm not sure how else to restate what I've already said.

[jayfoad]

I am in favour. It's pretty much what I suggested here: #115377 (comment)

[tschuett]

No GitHub approval?

[aemerson]

No GitHub approval?

@jayfoad already gave approval.

[tschuett]

In GitHub style, he commented. He didn't approve in GitHub style.

I am in favour. I don't read that as GitHub style approval.

[aemerson]

In GitHub style, he commented. He didn't approve in GitHub style.

I am in favour. I don't read that as GitHub style approval.

Yes, perhaps true. I would have done a direct commit but made this PR for feedback on the wording, not on the principle. If Matt or Jay disagree on a technical basis I'm more than happy to revert and listen to their opinions, they've always been respectful to me and are themselves great contributors to GISel.

Given that you've been blatantly rude and disrespectful towards me and my contributions to LLVM on other PRs, referred to me as a "bad reviewer" and a litany of earlier transgressions, I am in no mind to pay any attention to your opinions on any matter whatsoever, especially given that I'm responsible for many major GlobalISel features and optimizations in the past.

I have a busy job and young children, and I do code reviews in my spare time in order to help the project continue and unblock other contributors. Unfortunately I no longer have time to write many patches myself and therefore don't satisfy your idea of "exciting combiner contributions". Yet I have never stooped to insulting you on a personal level and have tried many times to provide feedback to help you understand how to make higher quality contributions.