This is an archive of the discontinued LLVM Phabricator instance.

[X86] Enable the VR512 register class when AVX512 is enabled
AbandonedPublic

Authored by n-omer on Jun 30 2023, 7:51 AM.

Details

Summary

This fixes https://github.com/llvm/llvm-project/issues/63615 via TargetLowering::getRegForInlineAsmConstraint() but appears to cause regressions.

I'm currently dealing with the failing tests and crashes.

Diff Detail

Event Timeline

n-omer created this revision.Jun 30 2023, 7:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2023, 7:51 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
n-omer requested review of this revision.Jun 30 2023, 7:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2023, 7:51 AM
pengfei requested changes to this revision.Jun 30 2023, 8:23 AM

I think this is not the right fix. We intended to define legal types under useAVX512Regs.
I think a proper fix should be setting "min-legal-vector-width"="512" when parsing the inline asembly in FE.

This revision now requires changes to proceed.Jun 30 2023, 8:23 AM

I think this is not the right fix. We intended to define legal types under useAVX512Regs.

I agree with you that this specific fix may not be the right but at the moment when "asked" whether 512 bit types are legal the backend "says" no even though AVX512 is explicitly enabled (see TargetLowering::getRegForInlineAsmConstraint for concrete details), this appears to be the cause of this bug.

I think a proper fix should be setting "min-legal-vector-width"="512" when parsing the inline asembly in FE.

I'm not at all familiar with Clang which happens to the front-end in this case but wouldn't the front-end have to ask the backend for this information?
IIUC the first time inline assembly is really scrutinized is during DAG construction in SelectionDAGBuilder::visitInlineAsm (please correct me if I'm wrong), and that is the point at which we try to discover which registers are used in the inline assembly and which registers are clobbered by it so that they can be appropriately represented in the DAG. This is where TargetLowering::getRegForInlineAsmConstraint comes into play and things go wrong.

This comment was removed by craig.topper.

I think this is not the right fix. We intended to define legal types under useAVX512Regs.

I agree with you that this specific fix may not be the right but at the moment when "asked" whether 512 bit types are legal the backend "says" no even though AVX512 is explicitly enabled (see TargetLowering::getRegForInlineAsmConstraint for concrete details), this appears to be the cause of this bug.

I think a proper fix should be setting "min-legal-vector-width"="512" when parsing the inline asembly in FE.

I'm not at all familiar with Clang which happens to the front-end in this case but wouldn't the front-end have to ask the backend for this information?
IIUC the first time inline assembly is really scrutinized is during DAG construction in SelectionDAGBuilder::visitInlineAsm (please correct me if I'm wrong), and that is the point at which we try to discover which registers are used in the inline assembly and which registers are clobbered by it so that they can be appropriately represented in the DAG. This is where TargetLowering::getRegForInlineAsmConstraint comes into play and things go wrong.

MS inline assembly is fully parsed during the frontend to detect the registers that are written. This adds the ~{zmm6} you can see on the statement in IR.

CGStmt.cpp in the frontend updates LargestLegalVectorWidth and thus min-legal-vector-width for inline assembly inputs and outputs, but I missed clobbers.

n-omer added a comment.Jul 3 2023, 1:46 AM

Thanks @pengfei and @craig.topper, I'll close this reivew.

n-omer abandoned this revision.Jul 3 2023, 1:48 AM