Page MenuHomePhabricator

[AMDGPU] Inline asm: use AsmNames for register constraints.
AbandonedPublic

Authored by SamWot on Apr 14 2016, 8:21 AM.

Details

Diff Detail

Event Timeline

SamWot updated this revision to Diff 53727.Apr 14 2016, 8:21 AM
SamWot retitled this revision from to [AMDGPU] Inline asm: use AsmNames for register constraints..
SamWot updated this object.
SamWot added reviewers: tstellarAMD, nhaustov.
SamWot added a project: Restricted Project.
SamWot added a subscriber: arsenm.
arsenm added inline comments.Apr 14 2016, 8:22 AM
lib/Target/AMDGPU/SIRegisterInfo.cpp
15

Should not be first include.

Why is SIRegisterInfo including from the InstPrinter? This seems like a layering violation

SamWot updated this revision to Diff 53731.Apr 14 2016, 8:36 AM
SamWot added inline comments.
lib/Target/AMDGPU/SIRegisterInfo.cpp
15

SIRegisterInfo needs access to AMDGPUInstPrinter::getRegisterName() to obtain AsmNames for registers. This is the only way to get it currently without changing core LLVM.
See this review: http://reviews.llvm.org/D19010

nhaustov accepted this revision.Apr 18 2016, 2:01 AM
nhaustov edited edge metadata.

Looks good.

inline-reg-constraints.ll -> inline-asm-reg-constraints.ll

This revision is now accepted and ready to land.Apr 18 2016, 2:01 AM
tstellarAMD requested changes to this revision.Apr 18 2016, 7:23 AM
tstellarAMD edited edge metadata.

Matt's concern about there being a layer violation is valid. We need to resolve this before the patch can be committed.

This revision now requires changes to proceed.Apr 18 2016, 7:23 AM

Is layering violation a strong prohibition for acceptance?
There is formal layering violation that RegisterInfo calls InstPrinter but for me it looks like it is conceptual layering violatoin in LLVM architecture. Assembly is responsibility of MC Layer but inline assembly is responsibility of TargetLowering and RegisterInfo (TargetLowering::getRegForInlineAsmConstraint() and RegisterInfo::getRegAsmName()).
So without code dupplication or major changes in core LLVM I don't see how this violation can be solved.
Possibly we should get used to that we must use names like VGPR13 or rename our Register classes.

It seems strange to me that this is here, instead of someplace like MCAsmInfo. Why was getRegAsmName added to TRI?

getRegAsmName() is used by TargetLowering::getRegForInlineAsmConstraint(). Placing it in MCAsmInfo will introduce layering violation in the same way.

What should I do with this change?
I don't see ways to solve layering violation without changes in core LLVM.

tstellarAMD accepted this revision.Apr 26 2016, 6:04 PM
tstellarAMD edited edge metadata.

What should I do with this change?
I don't see ways to solve layering violation without changes in core LLVM.

The easiest way to avoid a potential layering violation would be to have TableGen emit AMDGPUInstPrinter::getRegisterName(Reg) as static function rather than an AMDGPUInstPrinter member function.

However, if including the InstPrinter headers from the CodeGen files is a layering violation, all targets are already doing this by including InstrPrinter headers in XXXAsmPrinter. So, this change really isn't doing anything that isn't already being done. I'm fine with this being committed.

This revision is now accepted and ready to land.Apr 26 2016, 6:04 PM

Looks like patch was not committed.

SamWot abandoned this revision.Sep 30 2016, 2:07 AM