Fraser reported another problem with this patch to me privately. In addition to the wrong number of bits being written Craig found, we could also construct illegal instruction encodings. This could happen because we changed the VLMUL of the splat, but did not change the LMUL on the pseudo itself. As a result, the register allocator would assign e.g. an LMUL1 register, when the VSETVLI was now e.g. LMUL2. Given only have of registers are legal operands at LMUL2, this has a high chance of producing an illegal instruction encoding.

Update per review comments.

LGTM

Please land the new tests, and then rebase so that code changes are visible. (Unless this is fixing a crash, and then please clarify the commit message.)

Remove stray include change

Code is fine, but no tests?

Can I ask you to separate the various construction to fold changes into a separate NFC? I'd like to see that land and bake for a couple days before the removal itself goes in.

LGTM, but can you make sure to post a follow up patch to update cost model please?

LGTM

Direction looks reasonable, a couple comments.

In D128286#3605455, @craig.topper wrote:

In D128286#3605390, @reames wrote:

Warning, I may be miss understanding the problem you're solving here, but...

The case you mention appear to be specific to when VLEN=32 right? If so, a cleaner way of phrasing the illegal cases would seem to be to compute the effective vector length after LMUL (i.e. VLEN/8 for mf8), and then disallow any case where the implied vector length is shorter than a single element of the element type.

The problem is still valid for VLEN>=64 ELEN=32. While you are correct that a SEW=8 LMUL=1/8 would fit in a register for that config, there is no requirement in the spec for hardware to support it. The spec says that that the smallest fractional LMUL is SEWMIN/ELEN. Which would be 8/32 or 1/4 in this config. The spec goes on to say "For a given supported fractional LMUL setting, implementations must support SEW settings between SEWMIN and LMUL * ELEN, inclusive." So if ELEN is 32, LMUL=1/4 is only required to support SEW=8 and LMUL=1/2 is only required to support SEW=8 or 16.

I think I can still phrase that in the "does this fit" manner. I just need to introduce a MINVLEN (which is simply ELEN), and ask whether an implied vector length for a given LMUL contains a least one element.

Warning, I may be miss understanding the problem you're solving here, but...

LGTM, good find.

LGTM w/one required change.

Hm, this adds quite a bit of complexity.

Rebase over newly added tests - after thinking about this a bit more, I realized this was exerciseable through CostModel.

For the record, the minimal test effected by this was:
void e(int *argv[], int *p) {

for (int i = 0; i < 1024; i++)
  argv[i] = p;

}

Ok, with the revised description, let me start anew in my response.

@craig.topper Much clearer, thank you.

In D128155#3598053, @nikic wrote:

Logical and is select i1 %c1, i1 %c2, i1 false.

Also, IRBuilder::CreateLogicalAnd

In D126461#3597862, @craig.topper wrote:

dst in the patch description is not the pointer being loaded, it's the pointer of where to store the new_vl. That is only thing being checked for null in this patch.

I agree this is a possible interpretation, but it's also inconsistent with some of the other review discussion above.

ping

Went to land this today - I've been intentionally spacing out changes since I have a lot of churn in this file - and encountered a new crash in a test failure. Need to investigate and probably re-review once cause is understood.

Despite the comments above, the purpose of this patch remains unclear.

LGTM

Looks reasonable to me, though the TD changes are a bit more involved than I'm comfortable signing off on just yet. Will defer to Fraser.

Wrap code in macro to avoid release build warning.

LGTM w/minor optional comments.

Rework with the max-vl based approach we used in scatter/gather.