[PATCH] D18032: Power 9 Atomic instructions, load monitored and move XER to CR

[Nemanja Ivanovic via llvm-commits]

nemanjai added inline comments.

================
Comment at: lib/Target/PowerPC/PPCInstr64Bit.td:248
@@ +247,3 @@
+
+let hasExtraSrcRegAllocReq = 1 in
+def LDAT : X_RD5_RS5_IM5<31, 614, (outs g8rc:$rD), (ins g8rc:$rA, u5imm:$FC),
----------------
cycheng wrote:
> Depending on FC value, input might be $(rD+1), $(rD+2)
> I believe that's why we need "hasExtraSrcRegAllocReq".
> 
> I am curious about the effect of "hasExtraSrcRegAllocReq", looks like it disallows register changing in AggressiveAntiDepBreaker::ScanInstruction, because it is dangerous to do that for this instruction.
This is a good point, I added the comment to the 32-bit versions but forgot to add it here. It would certainly be a very bad idea for AADB to rename the **target** register when we may have values we need in the next register or two. All that being said, this should actually be **hasExtraDefRegAllocReq**. I'll fix it and re-post.

================
Comment at: lib/Target/PowerPC/PPCInstr64Bit.td:249
@@ +248,3 @@
+let hasExtraSrcRegAllocReq = 1 in
+def LDAT : X_RD5_RS5_IM5<31, 614, (outs g8rc:$rD), (ins g8rc:$rA, u5imm:$FC),
+                         "ldat $rD, $rA, $FC", IIC_LdStLoad>,
----------------
cycheng wrote:
> Do we need to check $FC value? Or it's user's responsibility?
> 
> Even though it looks like user's responsibility:
> > an Invalid function code will cause the system data storage error handler to be invoked.
I think that if we decide to provide access to this instruction through a builtin, we should diagnose invalid FC's supplied by the user. However, I don't think attempting to diagnose it in the .td files is the right way to go.

================
Comment at: lib/Target/PowerPC/PPCInstr64Bit.td:920
@@ +919,3 @@
+def LDMX : XForm_1<31, 309, (outs g8rc:$rD), (ins memrr:$src),
+                   "ldmx $rD, $src", IIC_LdStLD, []>, isPPC64,
+           Requires<[IsISA3_0]>;
----------------
cycheng wrote:
> I don't know when we need to add "isPPC64", Does LDAT and STDAT also require this attribute?
You are correct, those instructions are actually 64-bit only instructions. Not just names for the same instructions but that use 64-bit registers rather than 32-bit ones. I'll be sure to add that.


Repository:
  rL LLVM

http://reviews.llvm.org/D18032