This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Add new target feature to fuse arithmetic and logic operations
ClosedPublic

Authored by evandro on Jan 10 2019, 5:40 PM.

Diff Detail

Event Timeline

evandro created this revision.Jan 10 2019, 5:40 PM
fhahn added a comment.Jan 11 2019, 8:35 AM

Would it be possible to have a MIR test here, which just runs the scheduler?

llvm/lib/Target/AArch64/AArch64Subtarget.h
324

Not related to this patch directly, but is there a reason hasFuseCryptoEOR() is missing here?

evandro marked 2 inline comments as done.Jan 11 2019, 8:43 AM

Would it be possible to have a MIR test here, which just runs the scheduler?

I tried to keep the existing pattern, but I could add one too. Should it be done now or later?

llvm/lib/Target/AArch64/AArch64Subtarget.h
324

Since Cyclone is the only target using it, I guess that it doesn't care about running the MachineScheduler. At least not in the public repository. Though in the associated test case it is enabled in the command line.

fhahn added a comment.Jan 11 2019, 8:57 AM

Would it be possible to have a MIR test here, which just runs the scheduler?

I tried to keep the existing pattern, but I could add one too. Should it be done now or later?

I think it would be good to get a MIR test in from the start as it is easier to see what's going, similar to https://github.com/llvm-mirror/llvm/blob/master/test/CodeGen/AArch64/misched-fusion-crypto-eor.mir .

evandro updated this revision to Diff 181300.Jan 11 2019, 9:40 AM
evandro marked an inline comment as done.

I'm afraid that I'll need more time to craft the MIR file. Then, I might as well do the same for the other fusion tests.

evandro updated this revision to Diff 181372.Jan 11 2019, 2:45 PM

Replaced the test case with a MIR file.

fhahn accepted this revision.Jan 12 2019, 10:17 AM

LGTM, thanks! It would be great if you could extend the test cases a bit more before committing, covering cases with shifted regs and cases where some SUs have multiple successors and not all of them should be fused.

llvm/lib/Target/AArch64/AArch64MacroFusion.cpp
342

This indent is off by one level I think.

This revision is now accepted and ready to land.Jan 12 2019, 10:17 AM
evandro marked an inline comment as done.Jan 14 2019, 12:48 PM
This revision was automatically updated to reflect the committed changes.