This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][GISel] and+or+shl => bfi
ClosedPublic

Authored by jroelofs on May 27 2021, 3:55 PM.

Details

Summary

This fixes a GISEL vs SDAG regression that showed up at -Os in 256.bzip2

In _getAndMoveToFrontDecode:

gisel:

and w9, w0, #0xff
orr w9, w9, w8, lsl #8

sdag:

bfi w0, w8, #8, #24

Diff Detail

Event Timeline

jroelofs created this revision.May 27 2021, 3:55 PM
jroelofs requested review of this revision.May 27 2021, 3:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2021, 3:55 PM
aemerson added inline comments.May 27 2021, 4:43 PM
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
2164

Aesthetic opinion but no big deal: it's nicer to use:

if (!Ty.isScalar())
  return false;
if (Size != 32 && Size != 64)
  return false;

and bring the unsigned Size = ... up to here.

2171

Can we avoid re-matching on Dst here? We already know that MI == a G_OR, so we can just match the individual operands.

jroelofs added inline comments.May 27 2021, 11:08 PM
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
2164

ooh, I like that better too.

2171

I considered that, but we're getting one more thing from the m_GOR() here: the fact that it matches both orders of its operands via that commutative check in BinaryOp_match::match(). Is there a clean way to write that inline, or should I add another matcher for it?

jroelofs updated this revision to Diff 348546.May 28 2021, 8:58 AM
jroelofs marked an inline comment as done and an inline comment as not done.May 28 2021, 9:10 AM
paquette added inline comments.May 28 2021, 12:15 PM
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
2158

Why is this a doxygen comment?

2190

I think emitInstr handles the constraining for you already:

MachineInstr *AArch64InstructionSelector::emitInstr(
...
  constrainSelectedInstRegOperands(*MI, TII, TRI, RBI);
  return &*MI;
}
llvm/test/CodeGen/AArch64/GlobalISel/select-bitfield-insert.ll
2

Is it possible to use a MIR testcase instead?

jroelofs added inline comments.May 28 2021, 12:25 PM
llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp
2158

Copy-pasta from an older version of the patch when it was on a helper function. I'll fix.

2190

👍

llvm/test/CodeGen/AArch64/GlobalISel/select-bitfield-insert.ll
2

IIUC, a MIR test case wouldn't show that we're doing as well as (and in some cases better than) SDAG, but I'm happy to convert it if you think that doesn't matter so much.

paquette added inline comments.May 28 2021, 12:26 PM
llvm/test/CodeGen/AArch64/GlobalISel/select-bitfield-insert.ll
2

Ah, okay, I see. I think it's fine either way.

aemerson accepted this revision.May 28 2021, 2:07 PM

LGTM with minor nits addressed, thanks.

llvm/test/CodeGen/AArch64/GlobalISel/select-bitfield-insert.ll
2

I'm ok with either kind of test, the .ll tests do have an advantage in that its easier to see what the semantics of the generated code are.

Should probably add -global-isel-abort=1 to the test though.

This revision is now accepted and ready to land.May 28 2021, 2:07 PM
jroelofs updated this revision to Diff 348585.May 28 2021, 2:19 PM
This revision was landed with ongoing or failed builds.Jun 17 2021, 12:53 PM
This revision was automatically updated to reflect the committed changes.