This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Reduce scalar load/store isel patterns to a single ComplexPattern. NFCI
ClosedPublic

Authored by craig.topper on Jun 2 2022, 5:35 PM.

Details

Summary

Previously we had 3 different isel patterns for every scalar load
store instruction.

This reduces them to a single ComplexPattern that returns the Base
and Offset. Or an offset of 0 if there was no offset identified

I've done a similar thing for the 2 isel patterns that match add/or
with FrameIndex and immediate. Using the offset of 0, I was also
able to remove the custom handler for FrameIndex. Happy to split that
to another patch.

We might be able to enhance in the future to remove the post-isel
peephole or the special handling for ADD with constant added by D126576.

A nice side effect is that this removes nearly 3000 bytes from the isel
table.

Diff Detail

Event Timeline

craig.topper created this revision.Jun 2 2022, 5:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2022, 5:35 PM
craig.topper requested review of this revision.Jun 2 2022, 5:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2022, 5:35 PM
reames added a comment.Jun 3 2022, 8:21 AM

This looks clearly better to me. A couple small comments, but plan to LGTM after that.

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
1866

Minor: You can combine these two blocks if written as:
if (Addr.getOpcode() == ISD::ADD ||

(Addr.getOpcode() == ISD::OR && isOrEquivalentToAdd(Addr.getNode()))
1897

Same minor code structure comment as above.

Given we repeat this, maybe add a stripAndAccumulate12BitOffset helper?

1908

Why does this case need to be restricted to frame index? I think you maybe copy pasted this and didn't adjust?

craig.topper added inline comments.Jun 3 2022, 8:26 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
1908

This was an attempt to preserve the original isel pattern. isOrEquivalentToAdd only handles frame index and the isel pattern used AddrFI.

I'm considering using the more powerful SelectionDAG::isBaseWithConstantOffset that uses computeKnownBits for OR instead of special casing FrameIndex.

reames accepted this revision.Jun 3 2022, 8:43 AM

LGTM w/minor comments addressed. I'm also fine if a couple get split into following patches at your discretion.

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
1908

Let's do that as a follow up. That way the code changes end up in their own review separate from what is otherwise mostly NFC.

Add a comment to that effect in this patch please.

llvm/lib/Target/RISCV/RISCVInstrInfo.td
1207–1208

Do we even need a separate FrameAddRegImm here? The addressing mode should be the same for FI vs non-FI.

Can we write something along the lines of AddRegImm AddrFI:$Rs, simm12:$imm12? Not sure of my tablegen syntax here.

This revision is now accepted and ready to land.Jun 3 2022, 8:43 AM
craig.topper added inline comments.Jun 3 2022, 8:48 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
1908

I wrote the patch to use SelectionDAG::isBaseWithConstantOffset and it does cause test changes to vararg.ll.

llvm/lib/Target/RISCV/RISCVInstrInfo.td
1207–1208

ComplexPatterns can't nest so AddRegImm AddrFI:$Rs, simm12:$imm12 wouldn't work.

This revision was landed with ongoing or failed builds.Jun 3 2022, 9:01 AM
This revision was automatically updated to reflect the committed changes.