[ARM] tSETEND needs IsThumb

[olista01]

Summary

This is the Thumb encoding, so the Requires list must include IsThumb.

No test because we happen to select the ARM one first, but that's just luck.

[olista01]

Updated patch with more context.

[rengolin]

Hi Oliver,

I don't understand this one. Can you clarify why it's ok to select the ARM one for thumb code, and if that's ok, what is the problem?

[olista01]

It's _not_ OK to select the Thumb version of this instruction in ARM mode, and that's what this patch is preventing. Normally, the T1I base class sets the Predicates list to [IsThumb], but the Requires class overrides the whole list rather than appending to it, so we need to explicitly include IsThumb in the list here. The ARM version of this instruction does have IsARM in the Predicates list, so doesn't get selected for Thumb targets.

We just happen to not hit this bug at the moment because the ARM version of the instruction appears before this one in the matcher table, but that isn't actually guaranteed by anything, and other patches that I'm working on cause the order to be reversed.

[rengolin]

In D39190#904274, @olista01 wrote:

It's _not_ OK to select the Thumb version of this instruction in ARM mode, and that's what this patch is preventing.

Ok, this makes more sense. :)

Normally, the T1I base class sets the Predicates list to [IsThumb], but the Requires class overrides the whole list rather than appending to it, so we need to explicitly include IsThumb in the list here.

That's ugly! But I agree it's an acceptable fix.

The ARM version of this instruction does have IsARM in the Predicates list, so doesn't get selected for Thumb targets.

So, -mthumb selects this one just because it's the only match, and the ARM selects the arm one, by accident, just because it's the first to match. Gotcha.

We just happen to not hit this bug at the moment because the ARM version of the instruction appears before this one in the matcher table, but that isn't actually guaranteed by anything, and other patches that I'm working on cause the order to be reversed.

Other than changing the order of the td file in a test, I can't see how this could ever be tested.

And since it's an obvious fix, LGTM. Thanks!

lib/Target/ARM/ARMInstrThumb.td

 }
 
 def tSETEND : T1I<(outs), (ins setend_op:$end), NoItinerary, "setend\t$end",
-                  []>, T1Encoding<0b101101>, Requires<[IsNotMClass]>, Deprecated<HasV8Ops> {
+                  []>, T1Encoding<0b101101>, Requires<[IsThumb, IsNotMClass]>, Deprecated<HasV8Ops> {
   bits<1> end;
   // A8.6.156
   let Inst{9-5} = 0b10010;