[PATCH] D58400: [AMDGPU] Implement AMDGPUMCInstrAnalysis

Wed Feb 20 08:15:41 PST 2019

arsenm added inline comments.

================
Comment at: test/MC/AMDGPU/branch-comment.s:9
+// BIN-NOT: loop_start:
+// BIN: s_branch 65535 // 000000000004: BF82FFFF <keep_symbol+0x4>
+
----------------
scott.linder wrote:
> arsenm wrote:
> > This should be printed as signed?
> 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.
These should probably be consistently extended

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58400/new/

https://reviews.llvm.org/D58400