[PATCH] D83667: [ARM] Fix IT block generation after Thumb2SizeReduce with -Oz

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp:577
     if ((MO.isRegMask() && MO.clobbersPhysReg(ARM::CPSR)) ||
-        (MO.isReg() && MO.isDef() && MO.getReg() == ARM::CPSR)) {
+        (MO.isReg() && MO.isDef() && MO.getReg() == ARM::CPSR && !MO.isDead())) {
       Pred.push_back(MO);
----------------
The definition of DefinesPredicate says this for it's doc:
  /// If the specified instruction defines any predicate
  /// or condition code register(s) used for predication, returns true as well
  /// as the definition predicate(s) by reference.
I'm not sure we can just exclude dead predicates. They can still clobber the predicate in a way that might not be legal, and it looks like ifconvert is trying to use it for this reasons - to detect where the predicate might get clobbered.

There is this comment from ifconvert:
    // FIXME: Make use of PredDefs? e.g. ADDC, SUBC sets predicates but are
    // still potentially predicable.
Perhaps we have to tackle this in there? Or at least provide a way to remove the dead cpsr from the instruction. Perhaps just clearing the cpsr use from T1 instructions in PredicateInstruction would be enough?


================
Comment at: llvm/test/CodeGen/Thumb2/constant-hoisting.ll:44
+; CHECK-V7M-NEXT:    addeq r0, r1
+; CHECK-V7M-NEXT:    addseq r0, #1
+; CHECK-V7M-NEXT:    bxeq lr
----------------
Having a T1 addseq inside an IT block isn't really legal I believe - it should become an T1 addeq (by dropping the cpsr def) or a T2 instruction. I think if you try outputting an object file and disassembling it, it would end up as a T1 addeq, but assembling this directly might give a thumb2 instruction?

That might not cause any bugs, per-se, both are correct. But is something we should probably try and get right, in the same way we strive to not be non-deterministic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83667