[ARM] Fix t2BF_LabelPseudo.isTerminator

[lenary]

Summary

This pseudo represents a label definition, so it doesn't finish a basic
block (and it must not be duplicated, and it doesn't insert any
instructions).

This was found with a downstream run with EXPENSIVE_CHECKS enabled, where
this pseudo is introduced and used.

[dmgreen]

I hadn't realized these were upstream. They probably needn't be without the rest of the pass. I believe internally these they are both terminators and non-terminators. In that they can be generated between two non terminators or between two terminators. That obviously doesnt make sense, and I think the whole thing probably needs to be rewritten. Thats not been very high priority though.

[lenary]

In D148331#4268092, @dmgreen wrote:

I hadn't realized these were upstream. They probably needn't be without the rest of the pass. I believe internally these they are both terminators and non-terminators. In that they can be generated between two non terminators or between two terminators. That obviously doesnt make sense, and I think the whole thing probably needs to be rewritten. Thats not been very high priority though.

Yeah, understanding this Pseudo is really only possible with the downstream code, and we only got the EXPENSIVE_CHECKS failures downstream, but I realised this code is upstream which is why I put the fix upstream. If you have a better idea for the patch, feel free to commandeer it, or have someone like @simonwallis2 do so.

[dmgreen]

This patch certainly seems like an improvement, but I'm not sure what happens if the pseudo is in a terminator position but isn't marked as a terminator. Providing that doesn't go wrong I have no objections. Either way in the long run it should probably be cleaned up to perhaps be a pair of different pseudos.

[lenary]

In D148331#4275175, @dmgreen wrote:

Either way in the long run it should probably be cleaned up to perhaps be a pair of different pseudos.

There are other solutions that have been added to LLVM, like preInstrSymbol and postInstrSymbol, which might be more apt now, depending on the intended semantics (though, this should still be right, based on the code that's upstream for how these are emitted. I don't have access to the downstream code any more, so I can no longer verify, and on the same basis, I likely need someone from Arm to commandeer this patch from me anyway.

llvm/lib/Target/ARM/ARMInstrThumb2.td

class t2BF<dag iops, string asm, string ops>
   let Inst{31-27} = 0b11110;
   let Inst{15-14} = 0b11;
   let Inst{12} = 0b0;
   let Inst{0} = 0b1;
 
   let Predicates = [IsThumb2, HasV8_1MMainline, HasLOB];
 }
 
+// This is a pseudo op for a label used by a branch future instruction
 def t2BF_LabelPseudo
   : t2PseudoInst<(outs ), (ins pclabel:$cp), 0, NoItinerary, []> {
-  let isTerminator = 1;
+  // Turns into just a label, rather than any instructions.
+  let Size = 0;
+  // This defines a label, $cp, so we shouldn't duplicate this because we cannot
+  // define the label in two different places.
+  let isNotDuplicable = true;
   let Predicates = [IsThumb2, HasV8_1MMainline, HasLOB];
   let hasNoSchedulingInfo = 1;
 }
 
 def t2BFi : t2BF<(ins bflabel_u4:$b_label, bflabel_s16:$label, pred:$p),
                  !strconcat("bf", "${p}"), "$b_label, $label"> {
   bits<4> b_label;
   bits<16> label;