Page MenuHomePhabricator

[RISCV][WIP] Move VSPILL/VRELOAD expansion for vector tuples to eliminateFrameIndex.
ClosedPublic

Authored by craig.topper on Dec 1 2022, 9:33 PM.

Details

Summary

We need a scratch GPR to increment the base pointer for each subsequent
register. We currently reuse the input GPR for the base pointer without
declaring it as a Def of the pseudo.

We can't add it as a Def of the pseudo at creation time because it doesn't
get register allocated. This was tried in D109405.

Seems the only choice we have is to scavenge the GPR. This patch
moves the expansion to eliminateFrameIndex where we can create
virtual registers that will be scavenged. This also eliminates the
extra operand for passing vlenb from frame lowering to expand pseudos.

I need to do more testing on real world code, but wanted to get this
up for early review.

I hope this will fix the issue reported in D123394, but I haven't
checked yet.

Diff Detail

Event Timeline

craig.topper created this revision.Dec 1 2022, 9:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2022, 9:33 PM
craig.topper requested review of this revision.Dec 1 2022, 9:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2022, 9:33 PM
reames added inline comments.Dec 5 2022, 12:48 PM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
276

Missing {} will cause this to not compile without assertions. Same in the function below.

On the surface, this seems not unreasonable. I think I'm missing a bit of context though, so may need to discuss offline before I can offer meaningful review.

craig.topper added inline comments.Dec 5 2022, 12:54 PM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
276

Why? I moved this from another function that didn't have braces and it has been compiling fine.

reames added inline comments.Dec 5 2022, 1:01 PM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
276

So, doesn't an assert expand to an empty string when assertions are enabled?

If so, we have a dangling else. The next statement effectively becomes the body of the else.

Looking at the code you copied from, this is really quite interesting as it looks like that code is just blatantly wrong. The key lowering loop would be conditional on the else. Do we have any testing for this? And if we do, does that test pass without asserts?

craig.topper added inline comments.Dec 5 2022, 1:05 PM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
276

Doesn't the ; that would still be there count as a statement for the else?

reames added inline comments.Dec 5 2022, 1:10 PM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
276

Yes it does. Apparently I need more coffee...

Sorry for wasting your time.

reames accepted this revision.Dec 6 2022, 10:22 AM

LGTM w/minor comment.

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
465–495

Can you add a comment above this? Something along the lines of:

Handle spill/fill of synthetic register classes for segment operations to ensure correctness in the edge case one gets spilled. There are many possible optimizations here, but given the extreme rarity of such spills, we prefer simplicity of implementation for now.

This revision is now accepted and ready to land.Dec 6 2022, 10:22 AM
This revision was landed with ongoing or failed builds.Dec 6 2022, 3:42 PM
This revision was automatically updated to reflect the committed changes.