Page MenuHomePhabricator

[ValueTypes] Define MVTs for v6i32, v6f32, v7i32, v7f32
ClosedPublic

Authored by critson on Jun 8 2021, 2:57 AM.

Diff Detail

Event Timeline

critson created this revision.Jun 8 2021, 2:57 AM
critson requested review of this revision.Jun 8 2021, 2:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2021, 2:57 AM

Awfully brave to add new non-pow2 types; I like it. Unfortunate that we've both filed patches to this same file at the same time (D103884). One of us will have a fun time.

foad added a subscriber: foad.Jun 8 2021, 5:46 AM

Awfully brave to add new non-pow2 types; I like it. Unfortunate that we've both filed patches to this same file at the same time (D103884). One of us will have a fun time.

Technically there's precedent with MVT::v3i32 but where does this end? What happens when someone next asks for v8f32 -> v15f32? How tricky would it be for a custom AMDGPU DAG node to encode the vector width instead and it just uses the next pow2 type?

Awfully brave to add new non-pow2 types; I like it. Unfortunate that we've both filed patches to this same file at the same time (D103884). One of us will have a fun time.

Feel free to commit. I'll rebase this on top of the new state.

Technically there's precedent with MVT::v3i32 but where does this end? What happens when someone next asks for v8f32 -> v15f32? How tricky would it be for a custom AMDGPU DAG node to encode the vector width instead and it just uses the next pow2 type?

Yes, the precedent is v3i32/v5i32.
I agree its a dangerous slope, but I would be happy if we declared v7 as the hard limit for non-pow2 types.
Declaring a hard limit may also benefit code that has to work with these?

On recent hardware we have features which mean there isn't a need for large contiguous vector types in common cases.
However we have to disable this beyond v5 on some common hardware for stability.
Supporting v6/v7 benefits all our hardware going back many generations (where benefit = reduced register pressure).
I can workaround the lack of v6/v7, but our consensus is that we should just add the MVT types (see D103800).

foad added a comment.Jun 9 2021, 1:44 AM

Technically there's precedent with MVT::v3i32 but where does this end?

Where does it end for power-of-two types? It seems like they get added whenever any target has a need for them. I don't see why non-power-of-two types would be fundamentally different in that regard.

critson updated this revision to Diff 350857.Jun 9 2021, 4:47 AM
  • Rebase
RKSimon accepted this revision.Jun 10 2021, 5:31 AM

OK, I get where everyone's coming from and I'm not going to block it - it just feels untidy to me. LGTM

This revision is now accepted and ready to land.Jun 10 2021, 5:31 AM
This revision was landed with ongoing or failed builds.Jun 10 2021, 4:59 PM
This revision was automatically updated to reflect the committed changes.
srj added a subscriber: srj.Jun 14 2021, 12:16 PM

This commit seems to have injected a failure for x86-32 codegen in Halide; for one of our tests involving vector reductions, this commit causes a new assertion failure: MachineValueType.h:469: llvm::MVT llvm::MVT::changeVectorElementType(llvm::MVT) const: Assertion VecTy.SimpleTy != MVT::INVALID_SIMPLE_VALUE_TYPE && "Simple vector VT not representable by simple integer vector VT!"' failed.`

I'll be adding more details and adding a repro case as soon as I have a small one; posting this now to see if anyone else has noticed a regression/injection.

srj added a comment.Jun 14 2021, 3:04 PM

I'll be adding more details and adding a repro case as soon as I have a small one; posting this now to see if anyone else has noticed a regression/injection.

I don't have a standalone repro case for you yet, but the assertion failure in MVT::changeVectorElementType(EltVT) has this = llvm::MVT::v6i32, EltVT = llvm::MVT::i16.

srj added a comment.Jun 14 2021, 3:46 PM

Debugging into this a little further, it's unclear to me how EVT::changeVectorElementType() can be expected to work reliably: it assumes that if EltVT.isSimple() == true, then MVT::changeVectorElementType(EltVT) will be legal, but this is clearly not the case, since there aren't equivalent widths of MVTs for all element types. (In this case, v6i32 -> v6i16 isn't possible since v6i16 doesn't exist).

But what really puzzles me is why this never broke before, since even before this change, this wasn't a valid assumption (e.g. v3i32 -> v3i8 would have failed similarly before) -- is there logic upstream of this call that intends to guard against this somehow?

srj added a comment.Jun 14 2021, 5:37 PM

I've gotten a repro case. Created https://bugs.llvm.org/show_bug.cgi?id=50709 with details. Please let me know if I should replicate details here instead.

srj added a comment.Jun 14 2021, 5:40 PM

Since there is a bug and a repro case, I'd like to ask if this change can be rolled back until a proper fix is in order.

Since there is a bug and a repro case, I'd like to ask if this change can be rolled back until a proper fix is in order.

It looks like @craig.topper fixed the bug with rG4017d0335a35334835bfae6fc3e258adcd9ed2dc so probably no need for a revert

Since there is a bug and a repro case, I'd like to ask if this change can be rolled back until a proper fix is in order.

It looks like @craig.topper fixed the bug with rG4017d0335a35334835bfae6fc3e258adcd9ed2dc so probably no need for a revert

Let me know, I am happy to revert if we need to investigate further as I have not committed anything depend on this yet.
The v6 types in particular do create some odd issues.