This is an archive of the discontinued LLVM Phabricator instance.

[X86][FastIsel] Teach how to select vector load instructions.
ClosedPublic

Authored by andreadb on Mar 25 2015, 6:33 AM.

Details

Summary

This patch teaches fast-isel how to select 128-bit vector load instructions.

I simply extended the switch statement in method 'X86FastEmitLoad' adding extra cases for 128-bit vector types.
I had to slightly modify the signature of 'X86FastEmitLoad' to allow selecting opcodes based on alignment information.

Added test CodeGen/X86/fast-isel-vecload.ll

Please let me know if ok to submit.

Thanks,
Andrea

Diff Detail

Event Timeline

andreadb updated this revision to Diff 22644.Mar 25 2015, 6:33 AM
andreadb retitled this revision from to [X86][FastIsel] Teach how to select vector load instructions..
andreadb updated this object.
andreadb edited the test plan for this revision. (Show Details)
andreadb added reviewers: qcolombet, ributzka, mkuper.
andreadb added a subscriber: Unknown Object (MLST).
andreadb updated this revision to Diff 22654.Mar 25 2015, 8:52 AM

Uploaded a new version of the patch.
This time with a correct test file that checks both SSE and AVX vector loads.

-Andrea

qcolombet added inline comments.Mar 25 2015, 9:58 AM
lib/Target/X86/X86FastISel.cpp
1099

By default, don’t we assume stuff are unaligned, i.e., Alignment = 1?

Hi Quentin,

lib/Target/X86/X86FastISel.cpp
1099

That's what I thought originally. However, when testing this patch, I noticed that without fast-isel I was always getting aligned load instructions.

Example:

define <4 x float> @foo(<4 x float>* %V) {
entry:
  %0 = load <4 x float>, <4 x float>* %V
  ret <4 x float> %0
}

without -fast-isel, llc -mattr=+sse2 generates a vmovaps.

As far as I understand, if a load doesn't specify the alignment, then the backend assumes the 'ABIAlignment' based on the value type.

This also matches the behavior of method 'X86FastISel::X86SelectStore'. At around line 934, we check for the alignment of the store in input; if a store node doesn't specify any alignment (i.e. Alignment is 0), then FastISel implicitly sets Alignment to the 'ABIAlignment'.
For the record: the code between lines 1096 and 1099 is mostly copy-pasted from that method.

In general I don't have a strong opinion on this: I can change line 1099 so that we propagate value 1 to Alignment. However, if we do this, then for consistency, we should probably also change how we check the alignment of store instructions in 'X86SelectStore'. What do you think?

qcolombet added inline comments.Mar 25 2015, 12:17 PM
lib/Target/X86/X86FastISel.cpp
331

Instead of passing a bool, I think the actual alignment we saw may be useful.
We could imagine that we generate different instructions based on the value of that alignment.

That being said, this is not the case right now, so we can adjust accordingly later. It just feel weird that the callers know about the required alignments.

What do you think?

1099

It seems we do the same for SelectionDAG, at least at some places, unlike what I thought.
May be worth bringing that up on the dev mailing list.

Anyhow, this part looks good as it is consistent with what exists.

hfinkel added inline comments.
lib/Target/X86/X86FastISel.cpp
1099

The general convention all throughout LLVM is that Alignment == 0 means to default to the required ABI alignment. You need Alignment == 1 to get completely unaligned.

andreadb added inline comments.Mar 25 2015, 12:38 PM
lib/Target/X86/X86FastISel.cpp
331

Right, I agree with you; it feels wrong that the caller sets the 'Aligned' flag without knowing which opcode will be selected for the load instruction.
I am going to replace 'bool Aligned' we 'unsigned Alignment'. I will then change the logic inside this method so that an 'aligned' load is selected only if Alignment is != 16.

Thanks!

andreadb updated this revision to Diff 22668.Mar 25 2015, 1:15 PM

Patch updated.
Method 'X86FastEmitLoad' now takes as input an unsigned 'Alignment' value (instead of a 'bool Aligned'). We now select aligned vector loads only if Alignment is >= 16.
Added extra test cases in CodeGen/X86/fast-isel-vecload.ll to check that we correctly select "ABI aligned" vector loads.

Please let me know what you think.

Thanks again for your time!
-Andrea

qcolombet accepted this revision.Mar 25 2015, 1:38 PM
qcolombet edited edge metadata.

Thanks Andrea!

LGTM.

@Hal, thanks for the clarifications.

Q.

This revision is now accepted and ready to land.Mar 25 2015, 1:38 PM

Thanks Quentin.
Committed revision 233270.

This revision was automatically updated to reflect the committed changes.