This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][CODEGEN] Added support of new inline assembler constraints
ClosedPublic

Authored by dp on Jun 11 2020, 6:54 AM.

Diff Detail

Event Timeline

dp created this revision.Jun 11 2020, 6:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2020, 6:54 AM
dp added a comment.Jun 22 2020, 5:30 AM

Any issues with this change? Thanks.

arsenm added inline comments.Jun 23 2020, 7:37 AM
llvm/docs/LangRef.rst
4151

This one is weird; we don't actually have any of these instructions?

llvm/lib/Target/AMDGPU/SIISelLowering.cpp
11065

Val & maskTrailingOnes(Size) might be clearer

dp marked 4 inline comments as done.Jun 23 2020, 10:14 AM
dp added inline comments.
llvm/docs/LangRef.rst
4151

Sure, we do not have anything like this in AMDGPU ISA encodings and I'm not sure how useful this constraint would be. However we should probably support it for the sake of compatibility with GCC.

llvm/lib/Target/AMDGPU/SIISelLowering.cpp
11065

Thanks! I was sure there is a function for this purpose in llvm libraries but was unable to find it. The name sounds a bit cryptic.

dp updated this revision to Diff 272762.Jun 23 2020, 10:15 AM
dp marked 2 inline comments as done.

Summary of changes:

  • simplified clearUnusedBits using maskTrailingOnes;
  • fixed a bug in constraint identification (isImmConstraint).
arsenm added inline comments.Jun 23 2020, 12:04 PM
llvm/docs/LangRef.rst
4151

I'm not sure we need to aim for perfect GCC compatibility. I mean why did they add all of these? Is anything using them? Should they have?

dp marked 2 inline comments as done.Jun 24 2020, 3:15 AM
dp added inline comments.
llvm/docs/LangRef.rst
4151

I have no idea. Some time ago I looked at GCC mail threads but only found a commit adding these constraint. There was no discussion, no rationale behind adding specific constraints.

So as we do not aim for 100% compatibility with GCC, I’ll update this change and remove support for ‘Kf’ and ‘L’ constraints.

arsenm added inline comments.Jun 24 2020, 7:04 AM
llvm/docs/LangRef.rst
4151

I would at least split the ones that we don't understand into a separate patch. I'm not entirely comfortable spreading features we don't understand

dp updated this revision to Diff 273284.Jun 25 2020, 3:37 AM
dp marked an inline comment as done.
dp edited the summary of this revision. (Show Details)

Support of constraints 'L' and 'Kf' has been separated as D82538.

arsenm accepted this revision.Jul 1 2020, 5:52 PM
This revision is now accepted and ready to land.Jul 1 2020, 5:52 PM
This revision was automatically updated to reflect the committed changes.