This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG] Add extra check on asm operand legalization.
Needs RevisionPublic

Authored by hliao on Mar 30 2021, 8:22 PM.

Details

Reviewers
arsenm
bogner
Summary
  • Besides checking whether the RC has legal type for the corresponding operand, need to check that type's legality from the whole target's point of view. That's because a specific type may be designated as a legal one per RC for certain instructions but it may not be generally legal in a target. In that case, we still need to legalize that value through bitcast so that the input and output of that inline asm is well prepared for its itself or its user.

Diff Detail

Event Timeline

hliao created this revision.Mar 30 2021, 8:22 PM
hliao requested review of this revision.Mar 30 2021, 8:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2021, 8:23 PM
hliao edited reviewers, added: bogner; removed: JustinBorb.Mar 30 2021, 8:24 PM
arsenm requested changes to this revision.Mar 31 2021, 3:57 PM

Missing test

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
8190–8191

I'm not sure I see what case this fixes exactly. The concept of type legality is so convoluted already.

llvm/lib/Target/AMDGPU/SIRegisterInfo.td
812

This is a separate change. All of the test changes are GLobalISel, and only from this part

This revision now requires changes to proceed.Mar 31 2021, 3:57 PM
hliao added inline comments.Mar 31 2021, 6:03 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
8190–8191

this change fixes the issue shown in llvm/test/CodeGen/AMDGPU/inline-asm.i128.ll. I was working on a similar issue reported but found your previous change removing i128 as a legal type.

hliao added a comment.Apr 9 2021, 6:50 AM

Missing test

llvm/test/CodeGen/AMDGPU/inline-asm.i128.ll is the exact test case for this issue and this fix. All test cases in this change just revert to the previous one. But, that newly added test is left intentionally as the regression of this fix.

arsenm added inline comments.Apr 9 2021, 1:19 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
8190–8191

So the point is to re-add i128 as a legal type?

I think I see where this is going, but it's hack on a hack. We want the type to occupy the registers, but we can't make them legal since we can't opt out of the consequences of doing so. This at least needs a comment explaining this

hliao added inline comments.Apr 9 2021, 5:24 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
8190–8191

But, there we really should check TLI.isTypeLegal() in addition to checking RC legality. The latter is specific to that inlined assembly and the former is target-wise. Both should be checked.