This is an archive of the discontinued LLVM Phabricator instance.

[NFC] [Hexagon] Simplify int8 truncation and validation
ClosedPublic

Authored by kimgr on Aug 13 2018, 1:26 AM.

Diff Detail

Event Timeline

kimgr created this revision.Aug 13 2018, 1:26 AM
kimgr added a comment.Aug 13 2018, 1:30 AM

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.

kimgr edited the summary of this revision. (Show Details)Aug 13 2018, 2:38 AM
kparzysz added a subscriber: llvm-commits.
kparzysz added inline comments.Aug 13 2018, 5:29 AM
lib/Target/Hexagon/HexagonBitSimplify.cpp
1980

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.

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 added inline comments.Aug 13 2018, 6:37 AM
lib/Target/Hexagon/HexagonBitSimplify.cpp
1980

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 added inline comments.Aug 13 2018, 7:18 AM
lib/Target/Hexagon/HexagonBitSimplify.cpp
1980

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

kimgr added inline comments.Aug 13 2018, 7:36 AM
lib/Target/Hexagon/HexagonBitSimplify.cpp
1980

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 added inline comments.Aug 13 2018, 7:37 AM
lib/Target/Hexagon/HexagonBitSimplify.cpp
1980

Yup.

kimgr updated this revision to Diff 160356.Aug 13 2018, 7:52 AM

Use llvm_unreachable to convince GCC that all is well.

kimgr added a comment.Aug 13 2018, 7:55 AM

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!

kparzysz accepted this revision.Aug 13 2018, 7:57 AM
This revision is now accepted and ready to land.Aug 13 2018, 7:57 AM
This revision was automatically updated to reflect the committed changes.