Page MenuHomePhabricator

[AArch64][GlobalISel] Make G_USUBO legal and select it
ClosedPublic

Authored by porglezomp on Jan 20 2021, 4:36 AM.

Details

Summary

The expansion for wide subtractions includes G_USUBO, so it's important
that it's a legal instruction.

Diff Detail

Event Timeline

porglezomp created this revision.Jan 20 2021, 4:36 AM
porglezomp requested review of this revision.Jan 20 2021, 4:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2021, 4:36 AM

Can you add selection tests for s32 and s64?

I think you can probably

  • Copy test/CodeGen/AArch64/GlobalISel/select-uaddo.mir -> test/CodeGen/AArch64/GlobalISel/select-usubo.mir
  • Replace G_UADDO with G_USUBO
  • Generate the check lines using utils/update_mir_test_checks.py

and that should cover it.

llvm/test/CodeGen/AArch64/GlobalISel/legalize-add.mir
78

I think I'd note in the commit message that you also improved testing for the other overflow ops. (Or, do this separately in a NFC commit.) Otherwise, someone 3 years down the line doing git history archaeology may be confused. :)

Add the requested selection tests

I added usubo/saddo/ssubo tests here by duplicating the tests in select-uaddo.

paquette accepted this revision.Jan 20 2021, 12:05 PM

LGTM, thanks!

This revision is now accepted and ready to land.Jan 20 2021, 12:05 PM

Since I don't have commit access, could you commit these for me?

This revision was automatically updated to reflect the committed changes.
hctim added a subscriber: hctim.Jan 22 2021, 2:16 PM

Hi, looks like this patch broke UBSan on aarch64:

Looks like it flips the condition on unsigned-integer-overflow:

volatile uint32_t a = 1;
volatile uint32_t b = 2;
volatile uint32_t c = a - b;
    17bc:	52800028 	mov	w8, #0x1
    17c0:	71000908 	subs	w8, w8, #0x2
prev:
    17c4:	54000142 	b.cs	17ec no_problems  // b.hs, b.nlast
    // fallthrough to __ubsan_handle_sub_overflow
now:
    17c4:	1a9f37e8 	cset	w8, cs  // cs = hs, nlast
    17c8:	360000c8 	tbz	w8, #0, no_problems
    // fallthrough to __ubsan_handle_sub_overflow

The reverse is also true, perfectly valid operands now fail:

int main() {
  volatile uint32_t a = 3;
  volatile uint32_t b = 2;
  volatile uint32_t c = a - b;
}
$ clang a.cpp -fsanitize=unsigned-integer-overflow
$ ./a.out
runtime error: unsigned integer overflow: 3 - 2 cannot be represented in type 'unsigned int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior

This was caught on our sanitizer Android buildbots: http://lab.llvm.org:8011/#/builders/77/builds/3082

This revision is now accepted and ready to land.Jan 22 2021, 4:50 PM

Fix instruction selection for G_USUBO

G_USUBO needs to be checked for overflow with the LO condition code instead of
the HS condition code. This should hopefully resolve the UBSan issue.

aemerson accepted this revision.Jan 22 2021, 5:18 PM

LGTM.

This revision was landed with ongoing or failed builds.Jan 22 2021, 5:30 PM
This revision was automatically updated to reflect the committed changes.