On 2/27/19 12:50 PM, Jiong Wang wrote:
> BPF XADD semantics require all Defs of XADD are dead, meaning any result of
> XADD insn is not used.
>
> However, BPF backend hasn't enabled sub-register liveness track, so when
> the source and destination operands of XADD are GPR32, there is no
> sub-register dead info. If we rely on the generic
> MachineInstr::allDefsAreDead, then we will raise false alarm on GPR32 Def.
> This was fine as there was no sub-register code-gen support for XADD which
> will be added by the next patch.
>
> To support GPR32 Def, ideally we could just enable sub-registr liveness
> track on BPF backend, then allDefsAreDead could work on GPR32 Def. This
> requires implementing TargetSubtargetInfo::enableSubRegLiveness on BPF.
>
> However, sub-register liveness tracking module inside LLVM is actually
> designed for the situation where one register could be splitted into more
> than one sub-registers for which case each sub-register could have their
> own liveness and kill one of them doesn't kill others. So, tracking
> liveness for each make sense.
>
> For BPF, each 64-bit register could only have one 32-bit sub-register. This
> is exactly the case which LLVM think brings no benefits for doing
> sub-register tracking, because the live range of sub-register must always
> equal to its parent register, therefore liveness tracking is disabled even
> the back-end has implemented enableSubRegLiveness. The detailed information
> is at r232695:
>
> Author: Matthias Braun <matze@xxxxxxxxxx>
> Date: Thu Mar 19 00:21:58 2015 +0000
> Do not track subregister liveness when it brings no benefits
>
> Hence, for BPF, we enhance MachineInstr::allDefsAreDead. Given the solo
> sub-register always has the same liveness as its parent register, LLVM is
> already attaching a implicit 64-bit register Def whenever the there is
> a sub-register Def. The liveness of the implicit 64-bit Def is available.
> For example, for "lock *(u32 *)(r0 + 4) += w9", the MachineOperand info
> could be:
>
> $w9 = XADDW32 killed $r0, 4, $w9(tied-def 0),
> implicit killed $r9, implicit-def dead $r9
>
> Even though w9 is not marked as Dead, the parent register r9 is marked as
> Dead correctly, and it is safe to use such information or our purpose.
>
> Signed-off-by: Jiong Wang <jiong.wang@xxxxxxxxxxxxx>
> ---
> lib/Target/BPF/BPFMIChecking.cpp | 103 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 102 insertions(+), 1 deletion(-)
>
> diff --git a/lib/Target/BPF/BPFMIChecking.cpp b/lib/Target/BPF/BPFMIChecking.cpp
> index a1e81cb..7a4ba96 100644
> --- a/lib/Target/BPF/BPFMIChecking.cpp
> +++ b/lib/Target/BPF/BPFMIChecking.cpp
> @@ -61,6 +61,107 @@ void BPFMIPreEmitChecking::initialize(MachineFunction &MFParm) {
> LLVM_DEBUG(dbgs() << "*** BPF PreEmit checking pass ***\n\n");
> }
>
> +// Make sure all Defs of XADD are dead, meaning any result of XADD insn is not
> +// used.
> +//
> +// NOTE: BPF backend hasn't enabled sub-register liveness track, so when the
> +// source and destination operands of XADD are GPR32, there is no sub-register
> +// dead info. If we rely on the generic MachineInstr::allDefsAreDead, then we
> +// will raise false alarm on GPR32 Def.
> +//
> +// To support GPR32 Def, ideally we could just enable sub-registr liveness track
> +// on BPF backend, then allDefsAreDead could work on GPR32 Def. This requires
> +// implementing TargetSubtargetInfo::enableSubRegLiveness on BPF.
> +//
> +// However, sub-register liveness tracking module inside LLVM is actually
> +// designed for the situation where one register could be splitted into more
> +// than one sub-registers for which case each sub-register could have their own
> +// liveness and kill one of them doesn't kill others. So, tracking liveness for
> +// each make sense.
> +//
> +// For BPF, each 64-bit register could only have one 32-bit sub-register. This
> +// is exactly the case which LLVM think brings no benefits for doing
> +// sub-register tracking, because the live range of sub-register must always
> +// equal to its parent register, therefore liveness tracking is disabled even
> +// the back-end has implemented enableSubRegLiveness. The detailed information
> +// is at r232695:
> +//
> +// Author: Matthias Braun <matze@xxxxxxxxxx>
> +// Date: Thu Mar 19 00:21:58 2015 +0000
> +// Do not track subregister liveness when it brings no benefits
> +//
> +// Hence, for BPF, we enhance MachineInstr::allDefsAreDead. Given the solo
> +// sub-register always has the same liveness as its parent register, LLVM is
> +// already attaching a implicit 64-bit register Def whenever the there is
> +// a sub-register Def. The liveness of the implicit 64-bit Def is available.
> +// For example, for "lock *(u32 *)(r0 + 4) += w9", the MachineOperand info could
> +// be:
> +//
> +// $w9 = XADDW32 killed $r0, 4, $w9(tied-def 0),
> +// implicit killed $r9, implicit-def dead $r9
> +//
> +// Even though w9 is not marked as Dead, the parent register r9 is marked as
> +// Dead correctly, and it is safe to use such information or our purpose.
> +static bool hasLivingDefs(const MachineInstr &MI,
> + const TargetRegisterInfo *TRI) {
> + const MCRegisterClass *GPR64RegClass =
> + &BPFMCRegisterClasses[BPF::GPRRegClassID];
> + std::vector<unsigned> GPR32LiveDefs;
> + std::vector<unsigned> GPR64DeadDefs;
> +
> + for (const MachineOperand &MO : MI.operands()) {
> + bool RegIsGPR64;
> +
> + if (!MO.isReg() || MO.isUse())
> + continue;
> +
> + RegIsGPR64 = GPR64RegClass->contains(MO.getReg());
> + if (!MO.isDead()) {
> + // It is a GPR64 live Def, we are sure it is live. */
> + if (RegIsGPR64)
> + return true;
> + // It is a GPR32 live Def, we are unsure whether it is really dead due to
> + // no sub-register liveness tracking. Push it to vector for deferred
> + // check.
> + GPR32LiveDefs.push_back(MO.getReg());
> + continue;
> + }
> +
> + // Record any GPR64 dead Def as some unmarked GPR32 could be alias of its
> + // low 32-bit.
> + if (RegIsGPR64)
> + GPR64DeadDefs.push_back(MO.getReg());
> + }
> +
> + // No GPR32 live Def, safe to return false.
> + if (GPR32LiveDefs.empty())
> + return false;
> +
> + // No GPR64 dead Def, so all those GPR32 live Def can't have alias, therefore
> + // must be truely live, safe to return true.
> + if (GPR64DeadDefs.empty())
> + return true;
> +
> + // Otherwise, return true if any aliased SuperReg of GPR32 is not dead.
> + std::vector<unsigned>::iterator search_begin = GPR64DeadDefs.begin();
> + std::vector<unsigned>::iterator search_end = GPR64DeadDefs.end();
> + for (auto I : GPR32LiveDefs) {
> + bool ActuallyDead = true;
> +
> + for (MCSuperRegIterator SR(I, TRI); SR.isValid(); ++SR) {
> + if (std::find(search_begin, search_end, *SR) == search_end) {
maybe you can just return true here?
> + ActuallyDead = false;
> + break;
> + }
> + }
> +
> + if (!ActuallyDead)
> + return true;
> + }
> +
> + return false;
> +}
> +
> void BPFMIPreEmitChecking::checkingIllegalXADD(void) {
> for (MachineBasicBlock &MBB : *MF) {
> for (MachineInstr &MI : MBB) {
> @@ -68,7 +169,7 @@ void BPFMIPreEmitChecking::checkingIllegalXADD(void) {
> continue;
>
> LLVM_DEBUG(MI.dump());
> - if (!MI.allDefsAreDead()) {
> + if (hasLivingDefs(MI, TRI)) {
> DebugLoc Empty;
> const DebugLoc &DL = MI.getDebugLoc();
> if (DL != Empty)
>
