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

[Ryan Taylor via Phabricator via llvm-commits]

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


================
Comment at: lib/Target/AMDGPU/MCTargetDesc/AMDGPUAsmBackend.cpp:10
 
+#include "SIInstrInfo.h"
 #include "MCTargetDesc/AMDGPUFixupKinds.h"
----------------
rtaylor wrote:
> arsenm wrote:
> > This is a layering violation
> Where would you like the InstrMapping to go? The others are here.
I suppose I could put it into Utils/AMDGPUBaseInfo.h which is already included in AMDGPUAsmBackend.cpp.... though that seems at first glance to also be a layering violation. Maybe it would be better if this was a mapping table, it would fit into AMDGPUBaseInfo.h  but I think InstrMapping is more fitting.


================
Comment at: lib/Target/AMDGPU/MCTargetDesc/AMDGPUAsmBackend.cpp:73-76
+  if (((int64_t(Value)/4)-1) == 0x3f)
+    return true;
+  else
+    return false;
----------------
arsenm wrote:
> return test();
Not sure what you mean here though the else is useless, so I'll remove that and just return false.


================
Comment at: lib/Target/AMDGPU/SOPInstructions.td:943-945
+  let Inst{47-32} = 0x0000;
+  let Inst{54-48} = 0x00;
+  let Inst{63-55} = 0x17f; //encoding
----------------
rampitec wrote:
> arsenm wrote:
> > I don't understand how this gets you a nop?
> I guess s_nop opcode is 0, simm is 0 and SOPP encoding is 0x17f.
> Maybe it would be more readable to write something like:
> 
> ```
> let Inst{63-55} = S_NOP.Inst{31-23}; // encoding
> let Inst{54-48} = S_NOP.Inst{22-16}; // opcode
> 
> ```
Yes, the opcode for s_nop is 0. The value I'm using is 0 and the encoding I had is the same as everywhere else but I can see how this makes it more understandable... maybe a simple comment here would help? I fixed this but it just makes tablegen less readable honestly, since now the S_NOP def has to go after the SOPP class but before the SOPP_w_nop encoding, so it's all alone among classes. Am I missing something? Prototyping would work but again, make everything less readable.


================
Comment at: lib/Target/AMDGPU/SOPInstructions.td:948
+
+class SOPP64 <bits<7> op, dag ins, string asm, list<dag> pattern = []> :
+  InstSI <(outs), ins, asm, pattern >, SOPPe64 <op>, Base_SOPP <asm> {
----------------
arsenm wrote:
> SOPP_w_nop or something? The current naming sounds like an actually different encoding (same with SOPPe64)
Sure, that's fine.


================
Comment at: test/MC/AMDGPU/offsetbug_once.s:9
+	s_nop 0
+        s_nop 0
+	s_nop 0
----------------
rampitec wrote:
> Another indent.
Thanks. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63494