[PATCH] D54896: Introduce control flow speculation tracking pass for AArch64.
Kristof Beyls via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 14 02:54:13 PST 2019
kristof.beyls marked 30 inline comments as done.
kristof.beyls added a comment.
In D54896#1354951 <https://reviews.llvm.org/D54896#1354951>, @zbrid wrote:
> Hi Kristof,
> Sorry about this late review. I wanted to make sure that I understood your implementation of SLH for ARM, so I took a look and made a few comments with some questions on this first patch. Let me know your thoughts. I'm planning to take a look at the other commit you made a few days ago related to SLH too. Hopefully I didn't make too many comments that were already addressed in the follow up commit.
Hi Zola,
I am very happy with you taking a look and asking those questions! Thank you!
I have tried to give clear answers to your questions. Please don't hold back in asking further if anything remains unclear.
================
Comment at: llvm/trunk/lib/Target/AArch64/AArch64SpeculationHardening.cpp:100
+
+#define AARCH64_SPECULATION_HARDENING_NAME "AArch64 speculation hardening pass"
+
----------------
zbrid wrote:
> Why is this a macro instead of a const variable?
I just followed existing practice in the other LLVM passes. Maybe this could be a const variable - not sure. If so, maybe best to make the change in a separate patch with a focus on making this change for all existing passes?
================
Comment at: llvm/trunk/lib/Target/AArch64/AArch64SpeculationHardening.cpp:164-166
+ // If both the true and the false condition jump to the same basic block,
+ // there isn't need for any protection - whether the branch is speculated
+ // correctly or not, we end up executing the architecturally correct code.
----------------
zbrid wrote:
> Just curious in general about this, are there cases where this is expected to happen in practice? It seems useless to have a branch that goes to the same place for both true and false.
This pass has seen quite a few iterations over the past couple of months. I'm sure I have seen examples of TBB == FBB in some cases while running the pass across a range of test codes, but I'm afraid I no longer remember where I've seen the example...
================
Comment at: llvm/trunk/lib/Target/AArch64/AArch64SpeculationHardening.cpp:172
+ // translate analyzeBranchCondCode to CondCode.
+ assert(analyzeBranchCondCode.size() == 1 && "unknown Cond array format");
+ CondCode = AArch64CC::CondCode(analyzeBranchCondCode[0].getImm());
----------------
zbrid wrote:
> How does this assert work? I don't understand how you're ANDing with the string
>
> ```
> "unknown Cond array format"
> ```
>
>
This is also an idiom used in LLVM. Anding with the string results in some documentation to be printed (the string) when the assert fires.
The string in the assert is often helpful as the actual condition check may be a bit cryptic if you hit an assert you haven't written yourself recently.
For more details, see https://llvm.org/docs/CodingStandards.html#assert-liberally.
================
Comment at: llvm/trunk/lib/Target/AArch64/AArch64SpeculationHardening.cpp:191
+ .addUse(AArch64::XZR)
+ .addImm(CondCode);
+ SplitEdgeBB.addLiveIn(AArch64::NZCV);
----------------
zbrid wrote:
> From the comments at the beginning of the file, I expected a CSDB instruction right after this instruction. Is that not needed or added somewhere else (like the follow up commit that adds the masking)?
The CSDB instruction is needed before the register containing the taint is actually used.
In this patch, no uses of the register containing the taint are introduced - that is done in the follow-on patch.
So, indeed, you'll see CSDB instructions being inserted as part of the follow-on patch - where uses of the taint register also get introduced.
================
Comment at: llvm/trunk/lib/Target/AArch64/AArch64SpeculationHardening.cpp:192
+ .addImm(CondCode);
+ SplitEdgeBB.addLiveIn(AArch64::NZCV);
+ }
----------------
zbrid wrote:
> What is a live in and why does this one get added?
The LiveIns record which registers are live at the start of the basic block.
Here, the CSEL instruction inserted at the start of the basic block uses (implicitly) the flags AArch64::NZCV. Those flags were set by the previous basic block, where the conditional jump lives that jumps to the current basic block. Therefore, here we have to explicitly mark that the flags are live at the start of the basic block.
================
Comment at: llvm/trunk/lib/Target/AArch64/AArch64SpeculationHardening.cpp:222
+ if (MBB.instr_end() != MBB.instr_begin())
+ DL = (--MBB.instr_end())->getDebugLoc();
+
----------------
zbrid wrote:
> Is this getting the instruction before the branch instruction? Then that instruction is used as the debug location for the FBB and TBB we got in lines 214/215? Did I understand this correctly?
instr_end() gets the "one past the last instruction" in the basic block, so "--MBB.instr_end()" gets the last instruction in the basic block MBB.
So, the instruction that is used as the debug location is the last instruction.
Presumably that last instruction will be the conditional branch instruction, so the tracking code (CSEL) will get the same debug info as the conditional branch instruction for which it's tracking miss-speculation.
================
Comment at: llvm/trunk/lib/Target/AArch64/AArch64SpeculationHardening.cpp:233
+ // Perform correct code generation around function calls and before returns.
+ {
+ SmallVector<MachineInstr *, 4> ReturnInstructions;
----------------
zbrid wrote:
> Why is this code in a separate code block?
There isn't really a need for it - it's just a personal style where I've used the block to indicate the conceptual extent of the "perform correct code generation around function calls and before returns".
I'll remove the nesting level in an update - there is indeed no need for it and it deviates from existing practice.
================
Comment at: llvm/trunk/lib/Target/AArch64/AArch64SpeculationHardening.cpp:262
+
+void AArch64SpeculationHardening::insertSPToRegTaintPropagation(
+ MachineBasicBlock *MBB, MachineBasicBlock::iterator MBBI) const {
----------------
zbrid wrote:
> Why doesn't this function return bool to indicate whether instructions were modified?
This function always modifies MBB - so there is no need to signal back to the caller whether instructions were modified.
================
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;
----------------
efriedma wrote:
> 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.
As Eli pointed to - you should be able to find the information in the location he pointed to.
For example, in section "C6.2.75 DSB", the details state "...SY ... Encoded as CRm = 0b1111."
================
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))
----------------
efriedma wrote:
> 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.
The xzr register exists in AArch64 ("64-bit Arm"), but not in ARM ("32-bit Arm").
You're understanding is otherwise correct. Let me quote the ArmARM ("Arm Architecture Reference Manual"):
"The name XZR represents the zero register for 64-bit operands where an encoding of the value 31 in the
corresponding register field is interpreted as returning zero when read or discarding the result when written."
A little bit of background on instruction aliases: quite a few "instructions" you write in assembly, like CMP are actually encoded as more general instructions - in this case SUBS.
So, the "CMP" instruction only really exists at the assembly level (for convenience) - but once encoded it is just a SUBS instruction.
To quote the ArmARM again on the CMP instruction:
"""
CMP <Xn|SP>, #<imm>{, <shift>}
is equivalent to
SUBS XZR, <Xn|SP>, #<imm> {, <shift>}
and is always the preferred disassembly.
"""
In MachineInstrs, these aliases are not available, only the "real" encodable instructions. So when creating an instruction, you need to use SUBS rather than CMP.
In the comments, I have tried to put in the mapping from the alias to the encodable instruction, as I personally find it handy.
I hope that makes sense?
================
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))
----------------
efriedma wrote:
> zbrid wrote:
> > Why not the following instead of the three above instructions?
> > ```
> > AND SP, SP, TaintReg
> > ```
> There is no such encoding.
As Eli said.
More general, only the following instructions can write to the SP register in AArch64: ADD, SUB, NEG, NEGS, ADDS, SUBS.
Therefore, to be able to AND the SP with another register and write that value to SP, we first need to move the SP to another register.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54896/new/
https://reviews.llvm.org/D54896
More information about the llvm-commits
mailing list