This is an archive of the discontinued LLVM Phabricator instance.

Teach the AArch64 backend to materialize immediates using a pair of ORR-immediate instructions.
ClosedPublic

Authored by resistor on Jan 3 2023, 10:25 PM.

Diff Detail

Event Timeline

resistor created this revision.Jan 3 2023, 10:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2023, 10:25 PM
resistor requested review of this revision.Jan 3 2023, 10:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2023, 10:25 PM
fhahn added a subscriber: fhahn.Jan 4 2023, 2:13 AM

Credit to czwarich for figuring out the algorithm to test for this.

Would be good to add a description.

HsiangKai added inline comments.
llvm/test/CodeGen/AArch64/arm64-movi.ll
321–322

Remove the FIXME.

390–391

Ditto.

resistor updated this revision to Diff 486458.Jan 4 2023, 8:27 PM

Refactor and add more code comments. Update test comments.

Updating D140952: Teach the AArch64 backend to materialize immediates using a pair of ORR-immediate

instructions.

Credit to czwarich for figuring out the algorithm to test for this.

Would be good to add a description.

I have attempted to document the approach very verbosely in the code. Are there specific parts where more detail would be helpful?

Hello. Feel free to add reviewers to a patch, otherwise people might not see it or think it is still WIP. The patch looks like a nice addition.

Can you clang-format the patch? I don't believe that ifs and returns should be on the same line.

There is another use of expandMOVImm in the MachineCombiner. Is that OK because it only generated single instructions? It should possibly just be generating a MOVi32imm/MOVi64imm instead.

llvm/lib/Target/AArch64/AArch64ExpandImm.cpp
308

Remove can

345

"as an AND of an OR"? or a maybe explain the inverse of the immediate thing. I can see why this would work, but find it difficult to follow - it would be good to add a longer comment explaining how it works.

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp
169

Can this happen?

resistor updated this revision to Diff 493198.Jan 29 2023, 9:42 PM
resistor marked an inline comment as done.

Update for review comments

Updating D140952: Teach the AArch64 backend to materialize immediates using a pair of ORR-immediate

instructions.

llvm/lib/Target/AArch64/AArch64ExpandImm.cpp
345

The ORR is a red herring. The initial ORR is just a move-immediate, which the following AND then uses as its LHS.

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp
169

I'm not aware that anything creates it today, but I thought it was better to have the API of be orthogonal, so as to minimize surprise for any future folks touching this area.

FYI, there's an incomplete patch D78981 which has misc two-instruction patterns for immediates, if you're looking in the area. I found when I was looking at it that the more obscure patterns trigger quite rarely in practice in the codebases I was looking at, though, so I never got around to completing it.

dmgreen accepted this revision.Jan 31 2023, 12:28 AM

Thanks. There are a couple of suggestions below, otherwise LGTM.

llvm/lib/Target/AArch64/AArch64ExpandImm.cpp
345

Yeah. Its not really "as a pair of ANDs of logical immediates". It might be better to say " as an AND of a pair of logical immediates"

llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp
169

An AND of 0 would just be a way of creating 0, and an AND would be a strange way of creating it. The Or with a WZR is the canonical form of a mov, but the And sounds like it would make less sense and might be a bug if it did come up. Perhaps change it to an assert that I->Op1 != 0 instead.

This revision is now accepted and ready to land.Jan 31 2023, 12:28 AM
resistor added inline comments.Feb 4 2023, 6:23 PM
llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp
169

That's not what's happening here. The 0 isn't in the immediate operand, but in the register number. It's a signaling mechanism that is used to tell the AArch64ExpandPseudos pass what to do.

dmgreen added inline comments.Feb 6 2023, 12:52 AM
llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp
169

But XZR is zero, so this is doing 0 & imm, which is always zero. So the result will always just be an awkward way of writing movi 0. It's not a big deal either way, but if we want a zero we should be using the more canonical ORRWrs that processors will recognize and optimize for. But if anything gets here it is likely a bug in AArch64_IMM::expandMOVImm.