[NFC] [Hexagon] Simplify int8 truncation and validation

[kimgr]

Summary

Silences a -Wuninitialized false positive from GCC 5.4.

[kimgr]

The original code should have worked (Opc was constrained to storerb, storerh or storeri by validation earlier in the method), but GCC complained:

llvm/lib/Target/Hexagon/HexagonBitSimplify.cpp:1989:16: warning: ‘V’ is used uninitialized in this function [-Wuninitialized]

Hopefully the new code handles truncation correctly -- ninja check-llvm passes for me.

[kparzysz]

lib/Target/Hexagon/HexagonBitSimplify.cpp

1980 ↗

(On Diff #160305)

These casts are actually necessary. Those are store-register instructions that take a 32-bit register and store the lower 8, 16 or 32 bits, and this code tries to replace them with the equivalent store-immediate instructions. For example, if the register contains 0x001000FF, the S2_storerb_io instruction will store 0xFF and can be replaced with a store-immediate of -1.

[kparzysz]

Try this:

name: fred
tracksRegLiveness: true
body: |
  bb.0:
    liveins: $r0
    %0:intregs = COPY $r0
    %1:intregs = A2_tfrsi 4351    ; 0x000010FF
    S2_storerb_io %0, 0, %1
...

llc -march=hexagon -run-pass hexagon-bit-simplify -o - bit-store-imm.mir

The output should be

%0:intregs = COPY $r0
S4_storeirb_io %0, 0, -1

[kimgr]

lib/Target/Hexagon/HexagonBitSimplify.cpp

1980 ↗

(On Diff #160305)

Oh, now I see what's going on... The truncation by int cast is a little subtle. Maybe transform U instead, something like:

switch (Opc) {
  case Hexagon::S2_storerb_io:
    U &= UINT8_MAX;
    break;
  case Hexagon::S2_storerh_io:
    U &= UINT16_MAX;
    break;
  case Hexagon::S2_storeri_io:
    U &= UINT32_MAX;
    break;
};

int64_t V = U;
if (!isInt<8>(V))
  return false;

?

[kparzysz]

lib/Target/Hexagon/HexagonBitSimplify.cpp

1980 ↗

(On Diff #160305)

That performs zero-extension, what we want is sign-extension: 0x000000FF needs to become 0xFFFFFFFF.

[kimgr]

lib/Target/Hexagon/HexagonBitSimplify.cpp

1980 ↗

(On Diff #160305)

Thanks, I see that now. I'm a little hesitant to suggest anything else, because I clearly have no idea what I'm doing :)

It turns out we can convince GCC using llvm_unreachable -- I think that's a nice surgical change:

// Only consider 8-bit values to avoid constant-extenders.
int V;
switch (Opc) {
  case Hexagon::S2_storerb_io:
    V = int8_t(U);
    break;
  case Hexagon::S2_storerh_io:
    V = int16_t(U);
    break;
  case Hexagon::S2_storeri_io:
    V = int32_t(U);
    break;
  default:
    llvm_unreachable("Unexpected store opcode");
}
if (!isInt<8>(V))
  return false;

That makes more sense, right?

[kparzysz]

lib/Target/Hexagon/HexagonBitSimplify.cpp

1980 ↗

(On Diff #160305)

Yup.

[kimgr]

Use llvm_unreachable to convince GCC that all is well.

[kimgr]

The title is probably no longer relevant, but I'm having trouble coming up with something apt. If this looks good, could you commit it for me with a reasonable message? Thanks!

llvm/trunk/lib/Target/Hexagon/HexagonBitSimplify.cpp

case Hexagon::S2_storerb_io:
       V = int8_t(U);
       break;
     case Hexagon::S2_storerh_io:
       V = int16_t(U);
       break;
     case Hexagon::S2_storeri_io:
       V = int32_t(U);
       break;
+    default:
+      // Opc is already checked above to be one of the three store instructions.
+      // This silences a -Wuninitialized false positive on GCC 5.4.
+      llvm_unreachable("Unexpected store opcode");
   }
   if (!isInt<8>(V))
     return false;
 
   MI->RemoveOperand(2);
   switch (Opc) {
     case Hexagon::S2_storerb_io:
       MI->setDesc(HII.get(Hexagon::S4_storeirb_io));