This is an archive of the discontinued LLVM Phabricator instance.

[llvm][sve] Reg + Imm addressing mode for ld1ro.
ClosedPublic

Authored by fpetrogalli on Jul 7 2020, 4:20 PM.

Diff Detail

Event Timeline

fpetrogalli created this revision.Jul 7 2020, 4:20 PM
sdesmalen added inline comments.Jul 20 2020, 9:16 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
12350

This change doesn't seem to be tested. Does this belong in a separate patch?

llvm/lib/Target/AArch64/SVEInstrFormats.td
7733

A few questions:

  • Should this pattern also use (PredTy PPR3bAny:$gp)?
  • Why 2 in AddedComplexity = 2 ?
  • Is AddedComplexity needed at all? (it already looks more 'complex' than the other pattern)

nit: i don't know why gp is used here instead of Pg, which is used in the rest of file.

fpetrogalli marked 2 inline comments as done.

Rename variables in tablegen patterns.

fpetrogalli marked 2 inline comments as done.Jul 20 2020, 2:19 PM
fpetrogalli added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
12350

This makes sure that the bfloat case in the test file will work only if the +bf16 is added to the target feature. If I remove this, the bfloat case will lower even without the +bf16 target feature.

To properly test this, I shall create an llc test that fails with a when +bf16 is not listed as a target feature and that catches the "missing patters" debug message from the failure. I don't think we have tests like these, but let me know if this is something you want me to add one.

FWIW, this "early exit if lowering bfloat when not targeting bfloat" code is used also in other places, like performST1Combine. It is needed because we have encoded only integer patterns in the tablegen files, and we lower FP values by bitcasting them to integer values.

llvm/lib/Target/AArch64/SVEInstrFormats.td
7733

Should this pattern also use (PredTy PPR3bAny:$gp)

Done.

Why 2 in AddedComplexity = 2 ?

I don't know, but neither no added complexity or AddedComplexity = 1 work. In both cases, the reg+reg addressing mode specified in the multicalss sve_mem_ldor_ss wins. AddedComplexity = 2 is the first non-zero value that makes it work...

nit: i don't know why gp is used here instead of Pg, which is used in the rest of file.

I made the class use $Pg. I guess I got $gp by copy-paste from other places (I can see few in this file).

sdesmalen added inline comments.Jul 22 2020, 8:23 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
12350

Ah okay, so it is kind of tested, i.e. when removing the +bf16 attribute the test would now fail, where it didn't previously. I don't think a special negative test is needed though.

12351

I think this should be cast, not static_cast.

llvm/lib/Target/AArch64/SVEInstrFormats.td
7733

Okay thanks for clarifying and fixing.

7734

nit: odd indentation.

fpetrogalli marked 4 inline comments as done.

Thank you for the review @sdesmalen, I have addressed all your comments.

Francesco

Adding some text here as phab doesn't allow me to submit empty comments!

sdesmalen accepted this revision.Jul 24 2020, 6:48 AM

LGTM, thanks!

This revision is now accepted and ready to land.Jul 24 2020, 6:48 AM
fpetrogalli marked an inline comment as done.Jul 24 2020, 9:19 AM
fpetrogalli added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
12351

reverting to static_cast as cast breaks the build

I replaced cast with static_cast to fix build failures.

fpetrogalli marked 2 inline comments as done.Jul 24 2020, 9:23 AM
This revision was automatically updated to reflect the committed changes.