This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Add patterns for zext/sext of shift amount.
ClosedPublic

Authored by efriedma on Dec 11 2018, 1:04 PM.

Details

Summary

Not sure this is the best fix, but it saves an instruction for certain constructs involving variable shifts.

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma created this revision.Dec 11 2018, 1:04 PM
t.p.northover added inline comments.Dec 12 2018, 3:03 AM
test/CodeGen/AArch64/shift-mod.ll
21–22 ↗(On Diff #177762)

There's no particular reason to use w8 here so this test is pretty fragile. In general I think we should never be hard-coding registers that aren't at an ABI boundary (and so that update_llc_test_checks.py is usually more trouble than it's worth).

update_llc_test_checks is a useful starting point because it generates the right checks to ensure you're checking the whole function, which is inconvenient to do by hand (trying to CHECK-NOT your way to checking the whole function is a bad idea).

I can replace the explicit reference to r8 with a regex, sure. (We could look at modifying update_llc_test_checks to generate a regex where appropriate, I guess... but it might be tricky to distinguish when a value is ABI-significant.)

update_llc_test_checks is a useful starting point because it generates the right checks to ensure you're checking the whole function, which is inconvenient to do by hand (trying to CHECK-NOT your way to checking the whole function is a bad idea).

I can see some benefit there, though when the tests do break I'm often annoyed by it checking irrelevant details (those comments in this case, for example). Either way I think we should very strongly discourage people from treating its output as good enough for final tests except in trivial one-instruction cases.

I can replace the explicit reference to r8 with a regex, sure.

Thanks.

(We could look at modifying update_llc_test_checks to generate a regex where appropriate, I guess... but it might be tricky to distinguish when a value is ABI-significant.)

I was musing along similar lines myself this morning. Intel vs AT&T (and use/def in general) would also be tricky; I suspect it would need to bake quite a bit of target-specific knowledge into it to get reasonable output.

I suspect it would need to bake quite a bit of target-specific knowledge into it to get reasonable output

Yes. But that's expected; assembly is inherently target-specific. There are some target-specific checks already.

efriedma updated this revision to Diff 181929.Jan 15 2019, 4:41 PM

Use regex instead of explicit "x8".

t.p.northover accepted this revision.Jan 16 2019, 2:13 AM

Thanks! LGTM.

This revision is now accepted and ready to land.Jan 16 2019, 2:13 AM
This revision was automatically updated to reflect the committed changes.