I've looked into this and there is no way to distinguish the case of hh (no warning) and ll (emit a warning). They are both equivalent: a user-defined composition vs. pre-existing subreg.

In general, looking at a single instruction is not sufficient to tell whether a given register becomes dead or not, so I wouldn't worry about the limitation you mentioned. Cases that we can reliably handle are (sub)ranges corresponding to the parts of the register that is given in the operand (i.e. the entire register if there is no subregister).

The conflict between user-defined and inferred compositions causes a warning in the SystemZ build:

warning: SubRegIndex SystemZ::subreg_h64 and SystemZ::subreg_h32 compose ambiguously as SystemZ::subreg_hh32 or SystemZ::subreg_h32

Ping.

Ping.

Ping.

It should be possible to move this loop out of the nest and only have it iterate over the preexisting patterns. Could you see if that generates different .inc files (it shouldn't)?

Ping.

Already implemented.

The fixes have been committed individually.

The practical application of this is to indicate aliasing with a given memory object, but without specifying exactly which part of the object is accessed (since it may be unknown). Dropping memory operands altogether will alias the operation with all other objects, so it will be correct, but overly conservative.

Yes, I do have one. I'll post it on Monday.

Thanks for finding all these things!

We should convert the copy to an IMPLICIT_DEF, but this patch is good as is.

Ping.

That warning can be eliminated by resolving such conflicts in favor of user-defined compositions.

The problem is in how tblgen calculates sub-registers. For a given register R, subreg indices of its sub-registers will be added to R, in other words a super-register "inherits" subreg indices from its subregisters. For example, V0 (from class VR128) has an explicit subreg index subreg_h64. It also has a sub-register F0D, which has a subreg index subreg_h32. As a result, V0 will have two subreg indices: subreg_h64 and subreg_h32. From the register structure (V0 vs F0D vs F0S) it is inferred that the composition V0.subreg_h64.subreg_h32 is same as V0.subreg_h32, a in general that the composition subreg_h64.subreg_h32 is equivalent to subreg_h32. At the same time, repeating this logic for a register F0Q from class FPR128 leads to adding subreg_h32 twice, which tblgen tries to resolve by referring to user-defined compositions. This is how subreg_hh32 shows up. However, now there is another result for composing subreg_h64.subreg_h32, hence the warning.

Looks good to me.

A bit of a background for clarity: <Something>ByHwMode represents <Something> parameterized by hardware mode. It's a map that to each hw mode assigns the corresponding value of <Something>. If a value for mode m is missing from the map, the default is used (this is a form of compression).

Will investigate.

This looks good. Do you have a testcase that demonstrates what this pass does?

Running tblgen with -debug shows

This situation is the same as the case of RAX vs EAX on X86. EAX is the lower half of RAX, but modifying EAX does not preserve the upper bits of RAX. On the other hand, modifying AX (lower half of EAX) does preserve the upper half of EAX. Originally, the former behavior was modeled for both cases, i.e. overwriting a lone subregister would be considered as overwriting the entire register. I added phony registers to X86 (e.g. HAX covering the upper half of EAX) to model the latter behavior for EAX, EBX, etc.

The option would be great. Then I could commit the fixes individually with the corresponding testcases.

I included the testcase changes into D50648, so this patch is no longer necessary.

This is just a demonstration of what I meant in the comments for llvm.org/PR38544: getting rid of explicit overlapping subregister indices solves the problem encountered in tc_subregliveness_noliveseg.ll.

Fixes for remaining testcases, except tc_subregliveness_noliveseg.ll. That one requires D50725.

Try this:

In the longer term we should probably make the error messages more self-explanatory.

Btw, I didn't see your update when writing my reply, but it confirms what I thought.

I understand your point, which I believe is that the user can always sneak in some invalid code using valid instructions, which then some pass will transform into an invalid instruction. This is actually motivated by such a case (an assert cause by user program). The problem is that the assert also helped find compiler bugs, so in that sense it served its purpose. This patch is meant to address the problem in the user code before it gets to that pass, or essentially to filter out user problems before they can no longer be differentiated from internal compiler errors. It isn't intended to catch all possible forms of invalid input, but I believe it covers the only way that the user's program can trigger that particular assertion.

Ping.

I am much more inclined to detect and handle invalid code early than to teach the whole backend to propagate it to the end. If a user decides to use an external assembler, code like that can trigger an error, while using internal assembler will simply hide it.

Fix the pattern to generate a load with the misaligned address? That already happens. The problem is that we have passes that make changes based on the assumption that the instructions are valid. We make no effort to gracefully handle obviously invalid code, and if we detect it, it is usually in an assert. I think that the trap is a compromise where it won't trigger any further problems during compilation and will still fail for the user.

What we can do it convert it to a trap and emit a warning.

This issue was detected in a customer code and it was actually a bug in it. We've decided to go this route to make it easier for customers to see what the problem is (and this scenario is almost guaranteed to be a source code bug).

I agree with Bjorn: both getSize and getPhysRegSize were not meant to be there. They were added because getting rid of them caused some OOT failures. That was a while back, so maybe they are no longer used. I'm all in favor of removing them, maybe we should just go ahead with it and see if anyone complains. I accepted D47199 since it contains the larger change.

Accepting as per comments in D50285.

Could you commit this to the 7.0.0 branch as well, if you haven't done that yet?

The crash seems to be gone. Thanks!