[GlobalISel] Describe undefined values for G_SBFX/G_UBFX operands

[bcahoon]

Summary

Describe the allowable values for the least-significant bit and width operands for
G_SBFX/G_UBFX. This is necessary when the values are run-time values (as opposed
to compile-time constants).

[foad]

Thanks for doing this. I think it's important that we reach agreement on these details.

I would suggest these rules (1):

G_SBFX, G_UBFX: 0 <= lsb < lsb + width <= bitwidth (all values unsigned)

This implies that width is not allowed to be 0.

Or as an alternative (2):

G_SBFX: 0 <= lsb < lsb + width <= bitwidth (all values unsigned)
G_UBFX: 0 <= lsb <= lsb + width <= bitwidth (all values unsigned)

This allows 0 width for G_UBFX only. The rationale is that value of 0-width unsigned field is trivially 0, but the value of a 0-width signed field is not well defined -- it could be either 0 or -1.

On balance I prefer (1) because (a) I think it's simpler to have the same rule for both opcodes, and (b) allowing "width" to take any value from 0 to bitwidth inclusive makes it more awkward to expand these operations into shifts and masking.

llvm/include/llvm/Target/GenericOpcodes.td

1382

As mentioned I think lsb should be strictly less than bitwidth, here and below.

[bcahoon]

Improved description of legal values, based upon review comments.

[bcahoon]

In D104245#2818751, @foad wrote:

Thanks for doing this. I think it's important that we reach agreement on these details.

I would suggest these rules (1):

G_SBFX, G_UBFX: 0 <= lsb < lsb + width <= bitwidth (all values unsigned)

This implies that width is not allowed to be 0.

Thanks for the suggestions! I updated the patch based upon the first suggestion. I agree that it is better for the reasons that you've described.

[foad]

LGTM assuming others agree with these rules - maybe wait a day in case they have comments.

[aemerson]

Shouldn't we also have verifier checks for these requirements in the statically known cases?

[foad]

In D104245#2825719, @aemerson wrote:

Shouldn't we also have verifier checks for these requirements in the statically known cases?

You mean, verify that the operand values are in range if they happen to be G_CONSTANTs? I don't think that's a good idea because constant-folding one of the operands could suddenly make the MIR fail verification.

[arsenm]

In D104245#2825719, @aemerson wrote:

Shouldn't we also have verifier checks for these requirements in the statically known cases?

No because the verifier should not be considering the context of the instruction. The operands are legal or not looking at just the instruction itself

llvm/docs/GlobalISel/GenericOpcode.rst

259

What what is the result if this is violated? Poison?

[bcahoon]

llvm/docs/GlobalISel/GenericOpcode.rst

259

I think poison makes sense. Though, I'm not sure how that concept occurs in the gMIR?

[foad]

llvm/docs/GlobalISel/GenericOpcode.rst

259

I think poison makes sense.

I think it should be the same as what you get from any out-of-range shift. In IR that would be poison. but...

Though, I'm not sure how that concept occurs in the gMIR?

... I get the impression that codegen does not really distinguish between undef and poison (yet).

Given that the description of shifts in this document does not mention the out of range case at all, I think the current patch is good to go as-is. Obviously it would be great if someone could add documentation about the out-of-range / undef / poison cases in future.

llvm/docs/GlobalISel/GenericOpcode.rst

 The source operands are registers as follows:
 
 - Source
 - The least-significant bit for the extraction
 - The width of the extraction
 
 G_SBFX sign-extends the result, while G_UBFX zero-extends the result.
 
+If the least-significant bit is larger than the number of bits in the
+source, the result is undefined. If the width is zero or if
+width minus the least-significant bit is larger than the number of bits
+in the source, the result is undefined.
+
 .. code-block:: none
 
   ; Extract 5 bits starting at bit 1 from %x and store them in %a.
   ; Sign-extend the result.
   ;
   ; Example:
   ; %x = 0...0000[10110]1 ---> %a = 1...111111[10110]
   %lsb_one = G_CONSTANT i32 1

llvm/include/llvm/Target/GenericOpcodes.td

def G_BZERO : GenericInstruction {
   let hasSideEffects = false;
   let mayStore = true;
 }
 
 //------------------------------------------------------------------------------
 // Bitfield extraction.
 //------------------------------------------------------------------------------
 
-// Generic signed bitfield extraction.
+// Generic signed bitfield extraction. If lsb is larger than the number of
+// bits in src, the result is undefined. If width is 0 or width - lsb is
+// larger than the number of bits in src, the result is undefined.
 def G_SBFX : GenericInstruction {
   let OutOperandList = (outs type0:$dst);
   let InOperandList = (ins type0:$src, type1:$lsb, type1:$width);
   let hasSideEffects = false;
 }
 
-// Generic unsigned bitfield extraction.
+// Generic unsigned bitfield extraction. If lsb is larger than the number of
+// bits in src, the result is undefined. If width is 0 or width - lsb is
+// larger than the number of bits in src, the result is undefined.
 def G_UBFX : GenericInstruction {
   let OutOperandList = (outs type0:$dst);
   let InOperandList = (ins type0:$src, type1:$lsb, type1:$width);
   let hasSideEffects = false;
 }
 
 //------------------------------------------------------------------------------
 // Optimization hints