This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Create VMOVRRD from adjacent vector extracts
ClosedPublic

Authored by dmgreen on Apr 10 2021, 4:24 AM.

Details

Summary

This adds a combine for extract(x, n); extract(x, n+1) -> VMOVRRD(extract x, n/2). This allows two vector lanes to be moved at the same time in a single instruction, and thanks to the other VMOVRRD folds we have added recently can help reduce the amount of executed instructions. Floating point types are very similar, but will include a bitcast to an integer type.

This also adds a shouldRewriteCopySrc, to prevent copy propagation from DPR to SPR, which can break as not all DPR regs extracted from directly. Otherwise the machine verifier is unhappy.

Diff Detail

Event Timeline

dmgreen created this revision.Apr 10 2021, 4:24 AM
dmgreen requested review of this revision.Apr 10 2021, 4:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 10 2021, 4:24 AM
SjoerdMeijer added inline comments.Apr 19 2021, 3:25 AM
llvm/lib/Target/ARM/ARMISelLowering.cpp
14115

Typo: another other?

14119

Out of curiousity, this means we recognise:

vmov.32 r3, d18[0]
vmov.32 r2, d18[1]

but not:

vmov.32 r2, d18[1]
vmov.32 r3, d18[0]

?

14119

Just out of u

dmgreen marked an inline comment as done.Apr 19 2021, 4:47 AM
dmgreen added inline comments.
llvm/lib/Target/ARM/ARMISelLowering.cpp
14119

It will create vmov r2, r3, d18, so should handle either. We are dealing with a DAG, so can start from the even lane and find the odd one. They don't have a fixed order (And we don't have fixed registers yet.)

dmgreen updated this revision to Diff 338483.Apr 19 2021, 4:52 AM

Fix typo.

This revision is now accepted and ready to land.Apr 19 2021, 5:19 AM
This revision was landed with ongoing or failed builds.Apr 20 2021, 7:16 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Apr 20 2021, 7:47 AM
thakis added inline comments.
llvm/lib/Target/ARM/ARMBaseRegisterInfo.h
216

This causes lots of

../../llvm/lib/Target/ARM/ARMBaseRegisterInfo.h:213:8: warning: 'shouldRewriteCopySrc' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
  bool shouldRewriteCopySrc(const TargetRegisterClass *DefRC,
       ^
../../llvm/include/llvm/CodeGen/TargetRegisterInfo.h:579:16: note: overridden virtual function is here
  virtual bool shouldRewriteCopySrc(const TargetRegisterClass *DefRC,
               ^
1 warning generated.

The override is likely intentional and we should just add override?

dmgreen added inline comments.Apr 20 2021, 7:51 AM
llvm/lib/Target/ARM/ARMBaseRegisterInfo.h
216

Ah, will fix! Thanks for the report.

I'm also fixing something else to do with this patch, to do with f64 not being legal.