This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][Global ISel] Add sext/zext improvements
ClosedPublic

Authored by dmgreen on Aug 16 2021, 8:29 AM.

Details

Summary

This patch adds improvements for sext/zext in Global ISel.

For example, this piece of code:

define i64 @si64(<4 x i32> %0, i32 %1) {
  %3 = extractelement <4 x i32> %0, i64 1
  %s = sext i32 %3 to i64
  ret i64 %s
}

Used to have this lowering:

si64:                            
	mov	s0, v0.s[1]
	fmov	w8, s0
	sxtw	x0, w8
	ret

Whereas this patch makes it lower to this:

si64:                             
	smov	x0, v0.h[0]
	ret

Diff Detail

Event Timeline

Rin created this revision.Aug 16 2021, 8:29 AM
Rin requested review of this revision.Aug 16 2021, 8:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2021, 8:29 AM
Rin edited the summary of this revision. (Show Details)Aug 16 2021, 8:35 AM
paquette added inline comments.Aug 16 2021, 9:47 AM
llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h
296

The boolean parameter on the end is true if this is commutative. I don't think G_EXTRACT_VECTOR_ELT is? Should this be false?

297

Can we use a name that makes this clear that this is matching G_EXTRACT_VECTOR_ELT? I can imagine people reading code using this thinking that it's matching some strange type of vector extend.

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
2148

Why did this code move?

llvm/test/CodeGen/AArch64/GlobalISel/arm64-atomic.ll
324

I don't think this test change is necessary.

Rin added inline comments.Aug 17 2021, 12:46 AM
llvm/include/llvm/CodeGen/GlobalISel/MIPatternMatch.h
296

Hmm, that's a good point. I need to look into that.

297

Yeah, sure thing, I'll rename it

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
2148

For the i64 case for G_SEXT we saw that it was choosing the tablegen patterns instead of the switch(Opcode). To avoid that, this code was moved to earlySelect.

llvm/test/CodeGen/AArch64/GlobalISel/arm64-atomic.ll
324

I think the test case was failing without the charge, but I'll switch it back and see if it works.

dmgreen added inline comments.Aug 17 2021, 12:54 AM
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
2148

Do we need to move all of this, or can we get away with just doing the i64 in earlySelect?

Rin added inline comments.Aug 17 2021, 12:56 AM
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
2148

I thought of that, but it would be a lot of code that gets repeated. But I can to do that instead

Rin updated this revision to Diff 366888.Aug 17 2021, 7:00 AM
Rin marked 3 inline comments as done.

Address review comments

Rin marked 2 inline comments as done.Aug 17 2021, 7:01 AM
Rin added inline comments.
llvm/test/CodeGen/AArch64/GlobalISel/arm64-atomic.ll
324

Nevermind, it seems this was only a change due to clang-format. I will get rid of it

dmgreen added inline comments.Aug 17 2021, 7:11 AM
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
3215

I think this needs to be matching any Integer, not the specific index of 1

3226

And then this needs to use that index found above, not 0.

llvm/test/CodeGen/AArch64/GlobalISel/legalise-sext-zext.mir
2

This should maybe be running -run-pass=instruction-select, not legalizer?

Rin updated this revision to Diff 367205.Aug 18 2021, 7:35 AM

Fix tests and address revie comments.

Rin marked 3 inline comments as done.Aug 18 2021, 7:35 AM
Rin updated this revision to Diff 367207.Aug 18 2021, 7:50 AM

Remove changes from atomic test.

Rin updated this revision to Diff 367208.Aug 18 2021, 7:53 AM

Remove changes from atomic test

paquette added inline comments.Aug 18 2021, 10:19 AM
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
3160

It looks like the code here and the code in the earlySelect case for G_SEXT is very similar.

Would it be possible to factor some of it out into functions?

I could imagine an emit function which represents this patter, which takes an IsSigned parameter. Then you could choose e.g. UMOV or SMOV based off of that.

llvm/test/CodeGen/AArch64/GlobalISel/legalise-sext-zext.mir
2

Can we rename this test to something like "select-extract-vector-elt-with-extend" or something? It's not really a legalisation test.

Rin updated this revision to Diff 367514.Aug 19 2021, 8:39 AM

Create function to avoid repeating code. Rename test.

This is my last day of placement with Arm so I'm handing this patch over.

Rin marked an inline comment as done.Aug 19 2021, 8:39 AM

Yep. Now that @Rin is off to do more important things, I'm happy to take over the patch and get it finished off.

I think some of the opcodes still need updating, which I will look into soon.

dmgreen commandeered this revision.Aug 23 2021, 10:46 AM
dmgreen edited reviewers, added: Rin; removed: dmgreen.
dmgreen updated this revision to Diff 368142.Aug 23 2021, 11:01 AM

Update with Opcodes and extended types. I split off the code into a selectUSMovFromExtend function to encapsulate it a bit more in the process.

dmgreen updated this revision to Diff 368625.Aug 25 2021, 6:47 AM

Update a missing test, and add an v8s8 equivalent.

aemerson added inline comments.Aug 26 2021, 11:22 AM
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
4907

Why are we trying to match a copy? These should have been combined away. If there are cases where they aren't, then we should probably manually match the pattern using a helper like getOpcodeDef() which can look through copies.

dmgreen added inline comments.Sep 6 2021, 9:31 AM
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
4907

My understanding is that the COPY is a FPR -> GPR COPY, so is still needed. We can change it to getOpcodeDef, but I preferred the m_GExtractVectorElt version :)

dmgreen updated this revision to Diff 370938.Sep 6 2021, 9:32 AM

Use getOpcodeDef

aemerson accepted this revision.Sep 6 2021, 10:27 PM

LGTM with nit.

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
4934

unfinished comment?

This revision is now accepted and ready to land.Sep 6 2021, 10:27 PM
This revision was automatically updated to reflect the committed changes.