This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Legalize MVT::i64x8 in DAG isel lowering.
ClosedPublic

Authored by labrinea on Jan 5 2021, 8:49 AM.

Details

Summary

This patch legalizes the Machine Value Type introduced in https://reviews.llvm.org/D94096 for loads and stores. It's notable to point out that the function that chooses an EVT for a given llvm type now needs to be specialized (dynamically binded) per target, as for AArch64 and only when the LS64 extension is present I am mapping [8 x i64] to MVT::i64x8, which would otherwise be MVT::Other since it's an aggregate type.

Diff Detail

Event Timeline

labrinea created this revision.Jan 5 2021, 8:49 AM
labrinea requested review of this revision.Jan 5 2021, 8:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 5 2021, 8:49 AM
Matt added a subscriber: Matt.Jun 24 2021, 11:06 AM
labrinea updated this revision to Diff 355935.Jul 1 2021, 10:35 AM
labrinea edited the summary of this revision. (Show Details)
labrinea added a reviewer: momchil.velikov.
t.p.northover added inline comments.Jul 8 2021, 2:30 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
8316

This looks like it turns all [8 x i64] IR types into i64x8, which is pretty risky.

At the very least it means we probably need to add new tests for any natural IR operation you can do to that type (load, store, extract, insert, alloca, GEP, phi, select, function calls & returns and anything else you can think of to throw at it).

efriedma added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
8316

To be clear, the risky part is making getValueType() do this universally, for every place where you can write [8 x i64] in LLVM IR. Adding a target hook specifically for inline asm operands should be fine.

labrinea updated this revision to Diff 360214.Jul 20 2021, 11:10 AM

Changes since last revision:

  • getAsmOperandValueType() is a target hook called only when the selection DAG is processing Inline Asm nodes
  • GlobalISel was crashing before and now it just falls back to Selection DAG.
efriedma added inline comments.Jul 26 2021, 3:11 PM
llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
347

It's a little unfortunate we're sticking this in target-independent code, but not sure there's a better place.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
251

Not sure why you're marking BITCAST Legal; there isn't any legal type to bitcast from.

254

I guess LOAD/STORE come out of type legalization?

labrinea added inline comments.Jul 27 2021, 2:09 AM
llvm/lib/CodeGen/GlobalISel/InlineAsmLowering.cpp
347

Not, I am afraid.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
251

I think that's a leftover from previous efforts, so as the UNDEF just below. I'll remove them.

254

These are the custom lowering operations for the load/store i512 instruction that clang inserts pre/post inline assembly for accessing the asm operands (see lines 4569 and 4595).

8319

This should be TargetLowering::getAsmOperandValueType. I'll fix it.

labrinea updated this revision to Diff 362010.Jul 27 2021, 6:39 AM

Changes in this revision:

  • Removed BITCAST and UNDEF from the legal operations on MVT::i64x8
  • Rectified the derived target hook getAsmOperandValueType() to call the parent as a fallback instead of getValueType()
labrinea edited reviewers, added: efriedma; removed: eli.friedman.Jul 29 2021, 3:08 PM
This revision is now accepted and ready to land.Jul 30 2021, 2:32 PM
This revision was landed with ongoing or failed builds.Jul 31 2021, 1:53 AM
This revision was automatically updated to reflect the committed changes.