[PATCH] D63494: [AMDGPU] Fix for branch offset hardware workaround

[Ryan Taylor via Phabricator via llvm-commits]

rtaylor marked 4 inline comments as done.
rtaylor added inline comments.


================
Comment at: lib/Target/AMDGPU/MCTargetDesc/AMDGPUAsmBackend.cpp:56-57
 
+static unsigned getRelaxedOpcode(const MCInst &Inst) {
+  unsigned Op = Inst.getOpcode();
+  switch (Op) {
----------------
arsenm wrote:
> I would much rather avoid the proliferation of junk opcodes for this. Can you use a bundle or another way to add an independent nop instruction?
I'm not sure that's possible, relaxInstruction is return the MCInst Res but I can't say for sure as I've never worked with bundles and I'm not sure how the compiler treats them. I agree though that this would be more ideal than having new 64 bit instructions.


================
Comment at: lib/Target/AMDGPU/SOPInstructions.td:1011-1018
+def S_CBRANCH_SCC0_64 : SOPP64 <
+  0x00000004, (ins sopp_brtarget:$simm16),
+  "s_cbranch_scc0 $simm16"
+>;
+def S_CBRANCH_SCC1_64 : SOPP64 <
+  0x00000005, (ins sopp_brtarget:$simm16),
+  "s_cbranch_scc1 $simm16"
----------------
arsenm wrote:
> If the opcodes end up unavoidable, the class should be fixed to generate the branch and the dummy at the same time, rather than requiring repeating each definition with the opcode value
I can have a look at doing this.


================
Comment at: test/MC/AMDGPU/offsetbug.s:18-20
+	tbuffer_load_format_x v0, v4, s[8:11],  format:22, 0 idxen offset:4
+	tbuffer_load_format_xyzw v[9:12], v4, s[8:11],  format:56, 0 idxen offset:8
+	tbuffer_load_format_xyzw v[13:16], v4, s[8:11],  format:56, 0 idxen offset:12
----------------
arsenm wrote:
> Can you use trivial instructions for sizes? I don't trust the size of most instructions to stay the same
Yes, I can. I can make them nops.


================
Comment at: test/MC/AMDGPU/offsetbug.s:82-83
+	s_nop 0
+        s_nop 0
+	s_nop 0
+	s_nop 0
----------------
arsenm wrote:
> Formatting
Ah, missed this, thanks.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63494