User Details
- User Since
- Oct 23 2013, 6:32 PM (453 w, 6 h)
Yesterday
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.
Tue, Jun 28
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.
Mon, Jun 27
LGTM, but can you make sure to post a follow up patch to update cost model please?
LGTM
Sat, Jun 25
Fri, Jun 24
Thu, Jun 23
Direction looks reasonable, a couple comments.
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.
Wed, Jun 22
LGTM w/one required change.
Hm, this adds quite a bit of complexity.
Tue, Jun 21
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.
Also, IRBuilder::CreateLogicalAnd
I agree this is a possible interpretation, but it's also inconsistent with some of the other review discussion above.
Mon, Jun 20
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.
Fri, Jun 17
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.
Thu, Jun 16
LGTM w/minor optional comments.
Rework with the max-vl based approach we used in scatter/gather.