[PATCH] D54896: Introduce control flow speculation tracking pass for AArch64.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/trunk/lib/Target/AArch64/AArch64SpeculationHardening.cpp:269-270
+    // insert full control flow speculation barrier (DSB SYS + ISB)
+    BuildMI(*MBB, MBBI, DebugLoc(), TII->get(AArch64::DSB)).addImm(0xf);
+    BuildMI(*MBB, MBBI, DebugLoc(), TII->get(AArch64::ISB)).addImm(0xf);
+    return;
----------------
zbrid wrote:
> Why do these instructions have an immediate operand? It looks like you're passing the default for ISB, but it's not clear to me what that number means for DSB. I checked this manual, but it doesn't say what the immediate operand is used for, only that it's limited in the value it can have: 
> 
>   - http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0802a/a64_general_alpha.html
>   - http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0802a/a64_general_alpha.html 
> 
> 
> 
The complete architecture reference manual is publicly available at https://developer.arm.com/docs/ddi0487/latest . This should allow you to quickly answer all your questions about the instruction choices.


================
Comment at: llvm/trunk/lib/Target/AArch64/AArch64SpeculationHardening.cpp:274
+
+  // CMP   SP, #0   === SUBS   xzr, SP, #0
+  BuildMI(*MBB, MBBI, DebugLoc(), TII->get(AArch64::SUBSXri))
----------------
zbrid wrote:
> Is having the zero register in ARM as the destination register equivalent to discarding the output or is the zero register affected?
> 
> If it's the former, why use CMP instead of SUBS if this instruction discards the result of the instruction in the same way that CMP does?
CMP is an alias.


================
Comment at: llvm/trunk/lib/Target/AArch64/AArch64SpeculationHardening.cpp:280
+      .addImm(0); // no shift
+  // CSETM x16, NE  === CSINV  x16, xzr, xzr, EQ
+  BuildMI(*MBB, MBBI, DebugLoc(), TII->get(AArch64::CSINVXr))
----------------
zbrid wrote:
> Why use CSINV instead of CSETM here?
CSINV is an alias.


================
Comment at: llvm/trunk/lib/Target/AArch64/AArch64SpeculationHardening.cpp:297
+
+  // mov   Xtmp, SP  === ADD  Xtmp, SP, #0
+  BuildMI(*MBB, MBBI, DebugLoc(), TII->get(AArch64::ADDXri))
----------------
zbrid wrote:
> Why use ADD instead of MOV?
MOV is an alias.


================
Comment at: llvm/trunk/lib/Target/AArch64/AArch64SpeculationHardening.cpp:298-314
+  BuildMI(*MBB, MBBI, DebugLoc(), TII->get(AArch64::ADDXri))
+      .addDef(TmpReg)
+      .addUse(AArch64::SP)
+      .addImm(0)
+      .addImm(0); // no shift
+  // and   Xtmp, Xtmp, TaintReg === AND Xtmp, Xtmp, TaintReg, #0
+  BuildMI(*MBB, MBBI, DebugLoc(), TII->get(AArch64::ANDXrs))
----------------
zbrid wrote:
> Why not the following instead of the three above instructions?
> ```
> AND SP, SP, TaintReg
> ```
There is no such encoding.


================
Comment at: llvm/trunk/lib/Target/AArch64/AArch64SpeculationHardening.cpp:338-339
+  MisspeculatingTaintReg = AArch64::X16;
+  TII = MF.getSubtarget().getInstrInfo();
+  TRI = MF.getSubtarget().getRegisterInfo();
+  bool Modified = false;
----------------
zbrid wrote:
> Why do you have to get this data every time the function is called? Could these be static since we wouldn't expect them to be different between the functions in the same compilation? Or are there cases where this would be different between two functions?
These can change between functions (with LTO, or certain function attributes).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54896