Together with my observation in D148316 and the new tests, this looks good.

@dmgreen Thanks for your review. Could you please merge this with "Lawrence Benson <github@lawben.com>".

Added check for illegal types. I could not get this check to actually fire. The type is legalized before the bitcast in the bitcast lowering case, and it is legalized before the
store becomes a truncating store. So in both cases other checks prevent bad things from happening. But it is probably still fine to keep it as a defensive check in case things
change.

Address a few review comments.

I dug into this a bit, and your vector compare sing bit patch does the right thing here. But the problem is unrelated and a bit annoying. At the time the sign_extend_inreg that we added is combined (to potentially remove it), there is a build_vector with (1 << vectorElementSize) - 1 bits (to negate via xor). But ComputeNumSignBits breaks here. The vector has 32-bit constants but we only have 8-bit vectors. So it detects 24 sign bits and then kinda gives up. So without changing code in SelectionDAG::ComputeNumSignBits or the negation of vector entries, there is no way to correctly determine the sign bits.

In D148316#4277254, @dmgreen wrote:

Does having sign bits for the vector compare nodes help, as in https://reviews.llvm.org/D148624? I wasn't able to find a good way to test that, the optimizer would always clear away the results for all the examples I tried prior to creating the compare nodes.

Add test for float vector. This required a single-line change to convert the VecVT to an integer vector for sign-extend to work.

Thanks for your effort in reviewing this patch. I think this solution is nicer than my original approach.

Changed approach as suggested by @dmgreen. We now use an explicit sign-extend and ignore the vector compare. The sign-extend is removed in later steps if there is a vector compare,
so there is no overhead. This change allows us to determine the original type in more cases, as we can detect both SETCC and TRUNC.

Addressed some review comments.

@dmgreen I can split this into two patches. I'll remove the truncate store part and only focus on the bitcast for now.

Changed large parts of where this conversion takes place.

Thanks. Could you please use "Lawrence Benson <github@lawben.com>".

@fhahn is there anything missing from your side? If not, could one of you or @dmgreen merge this? As this is my first patch, I obviously do not have push access :)

In D145301#4221490, @Sp00ph wrote:

In D145301#4221481, @lawben wrote:

If we have a comparison, we know that all bits are 1 or all bits a 0, so if the least significant one is equal to all others.

Aren't the elements in a <N x i1> guaranteed to be 0 or -1 (so all zeros or all ones) anyways? And even if there was always an extra instruction emitted so that for compare + bitcast the flow would look like this: <initial compare> -> <compare returned bitmask> -> <use and-trick on the result of that>, I would assume that LLVM would just trivially optimize out the second compare if it knows that the result of the first compare already contains all zeros/all ones.

@Sp00ph Your example would not be optimized. The issue with that example is: how is a bitcast to i1 defined? The current logic in LLVM uses the least significant bit. But this trick does not work in that case, as we use bits 0 to n for lanes 0 to n, so we only use the least significant one for lane 0. If we have a comparison, we know that all bits are 1 or all bits are 0, so the least significant one is equal to all others. Without a comparison, we could shift the least significant bit and then do the rest, but that would need an extra instruction. Maybe this could be added in a follow-up? I'm happy to discuss options here. The current approach is a bit more conservative.

@david-arm @dmgreen I rebased this into a single commit. I hope the changes are shown correctly now. All changes here are new, I did not modify any tests. I guess this was just shown incorrectly because I used multiple commits.

Rebase onto main

An honest attempt to do a rebase with arc :)

@dmgreen Okay, I rebased onto main and submited via arc. I hope this did the right thing, but it doesn't look broken. I guess my mistake in the other patch was to create a merge commit instead of rebasing. For future reference, I'll just always squash into in commit.

Rebase main

@dmgreen Thanks for the feedback. I've addressed the comments you made. I've renamed the method, as it does not require an AND anymore.

Add a path without AND mask.

Apply the bitcast to truncating stores.

Add tests for exit cases and extend to v8i8.

Sorry, this was meant to be an update to: https://reviews.llvm.org/D146212, not a new diff.

@t.p.northover As this is my first patch submitted to LLVM, this is just a short ping to check if there is something that I have missed or forgotten to do. I'm not yet familiar with the procedure.