This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Implement AMDGPUMCInstrAnalysis
ClosedPublic

Authored by scott.linder on Feb 19 2019, 11:36 AM.

Details

Summary

Implement MCInstrAnalysis for AMDGPU. Implement evaluateBranch to get <symbol+offset> notation in llvm-objdump when the symbolizer fails. I believe the remaining default implementations are OK, if pessimistic, but we can implement them as-needed.

Diff Detail

Event Timeline

scott.linder created this revision.Feb 19 2019, 11:36 AM
arsenm added inline comments.Feb 19 2019, 11:50 AM
lib/Target/AMDGPU/MCTargetDesc/AMDGPUMCTargetDesc.cpp
122

Needs a comment for why this is 18

test/MC/AMDGPU/branch-comment.s
10

This should be printed as signed?

16

Can you include some cases where it can't figure out an associated symbol?

16

Also could use some stressing the maximum branch distances. There's some macro trick one of the other tests uses to produce a lot of nops for this

scott.linder marked an inline comment as done.Feb 20 2019, 8:01 AM
scott.linder added inline comments.
test/MC/AMDGPU/branch-comment.s
10

I went back to make this change, but I'm not really certain why we choose to represent the branch immediate the way we currently do.

It seems like we reinterpret the bytes of a signed int16 as a signed int64 without a sign-extension when creating the MCOperand immediate. It seems confusing to me, and leads to some awkward casts in places. Would sign-extending and adding checks for isInt<16>(Imm) in places be a better approach, or is there something I'm missing?

Alternatively should we just make the asm parser/printer aware of this so we see "-1"? Should we also support the old way? It seems like there must be assembly kernels out there with the old "notation" for negative immediates.

arsenm added inline comments.Feb 20 2019, 8:15 AM
test/MC/AMDGPU/branch-comment.s
10

These should probably be consistently extended

scott.linder marked 4 inline comments as done.

Address feedback. Sign-extend all s_branch immediates and change the assembler syntax to represent these as true negative numbers.

One remaining question I have is about "overflow" in the calculation of the target; for example the test for the smallest simm16 on a branch at a low PC results in the offset <keep_symbol+0xfffffffffffe0018>but I don't know if the hardware is defined to behave this way. I will try to experiment, but I'm unsure how to know definitively.

arsenm added inline comments.Feb 20 2019, 2:03 PM
test/MC/AMDGPU/branch-comment-fail.s
4

This error message is bad. Why doesn't it say something about out of range?

scott.linder added inline comments.Feb 20 2019, 2:24 PM
test/MC/AMDGPU/branch-comment.s
16

I added tests for the boundaries when assembling an immediate, and tests for failure due to the immediate being out of range, but the test you mention (test/MC/AMDGPU/branch-comment-fail.s) already handles the symbolic case, so I don't think there is anything to add.

Improved error message.

Does anyone have an opinion on returning negative branch targets (e.g. <keep_symbol+0xfffffffffffe0018>)? I don't know how this would ever come up in hardware anyway, or what the hardware would do, but it doesn't seem very helpful in the disassembly.

Does anyone have an opinion on returning negative branch targets (e.g. <keep_symbol+0xfffffffffffe0018>)? I don't know how this would ever come up in hardware anyway, or what the hardware would do, but it doesn't seem very helpful in the disassembly.

The hardware doesn't know about the symbol? Do you mean for overflow?

Does anyone have an opinion on returning negative branch targets (e.g. <keep_symbol+0xfffffffffffe0018>)? I don't know how this would ever come up in hardware anyway, or what the hardware would do, but it doesn't seem very helpful in the disassembly.

The hardware doesn't know about the symbol? Do you mean for overflow?

Right, I just mean that the notation of an offset from a symbol kind of breaks down with the overflow, and that the "overflow" doesn't represent an overflow in the hardware anyway. At best <keep_symbol+0xfffffffffffe0018> is just an odd way to represent a negative offset. I think it makes more sense to just return nothing from evaulateBranch if the result would be negative?

After digging a bit more into how we parse/print operands for sopp_br I think there are some more fundamental decisions to make beyond just "do we support signed integers", so I want to avoid changing anything in this patch. I will revisit how we treat them in the future and make any breaking changes all at once, rather than spreading them out.

I'm not sure how the question is whether signed integers are supported. The instruction does treat the offset as signed

I'm not sure how the question is whether signed integers are supported. The instruction does treat the offset as signed

I mean during parsing/printing. We don't support parsing s_branch -1, nor we don't support printing it.

arsenm accepted this revision.Mar 1 2019, 3:29 PM

LGTM

This revision is now accepted and ready to land.Mar 1 2019, 3:29 PM
This revision was automatically updated to reflect the committed changes.