search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
27.18k stars 11.13k forks source link

scalar/integer miscompile from global isel for AArch64 #90245

Open regehr opened 3 months ago

regehr commented 3 months ago

here's a function:

define i32 @f(i32 %0, i32 %1) {
  %3 = sub i32 %1, %0
  %4 = sub nsw i32 %1, %0
  %5 = select i1 false, i32 %4, i32 %3
  %6 = icmp sgt i32 %5, 0
  %7 = select i1 %6, i32 0, i32 1
  ret i32 %7
}

using this driver:

#include <stdio.h>

unsigned f(unsigned, unsigned);

int main(void) {
  printf("%x\n", f(0xe, 0x80000001));
}

it should print 0, and this is what happens with the sdag backend, but using the global isel backend it prints 1.

for the given inputs, %4 is poison. the poison is blocked by the subsequent select but it seems like this is confusing the backend somehow, since llvm-reduce was unable to remove the poison-producing operation and the subsequent select.

cc @hatsunespica

llvmbot commented 3 months ago

@llvm/issue-subscribers-backend-aarch64

Author: John Regehr (regehr)

here's a function: ```llvm define i32 @f(i32 %0, i32 %1) { %3 = sub i32 %1, %0 %4 = sub nsw i32 %1, %0 %5 = select i1 false, i32 %4, i32 %3 %6 = icmp sgt i32 %5, 0 %7 = select i1 %6, i32 0, i32 1 ret i32 %7 } ``` using this driver: ```c #include <stdio.h> unsigned f(unsigned, unsigned); int main(void) { printf("%x\n", f(0xe, 0x80000001)); } ``` it should print 0, and this is what happens with the sdag backend, but using the global isel backend it prints 1. for the given inputs, `%4` is poison. the poison is blocked by the subsequent select but it seems like this is confusing the backend somehow. cc @hatsunespica
v01dXYZ commented 3 months ago

Hi,

The combiner does something strange I don't understand. Maybe related to poison value and select.

# *** IR Dump After IRTranslator (irtranslator) ***:
# Machine code for function f: IsSSA, TracksLiveness
Function Live Ins: $w0, $w1

bb.1 (%ir-block.2):
  liveins: $w0, $w1
  %0:_(s32) = COPY $w0
  %1:_(s32) = COPY $w1
  %4:_(s1) = G_CONSTANT i1 false
  %6:_(s32) = G_CONSTANT i32 0
  %9:_(s32) = G_CONSTANT i32 1
  %2:_(s32) = G_SUB %1:_, %0:_
  %3:_(s32) = nsw G_SUB %1:_, %0:_
  %5:_(s32) = G_SELECT %4:_(s1), %3:_, %2:_
  %7:_(s1) = G_ICMP intpred(sgt), %5:_(s32), %6:_
  %8:_(s32) = G_SELECT %7:_(s1), %6:_, %9:_
  $w0 = COPY %8:_(s32)
  RET_ReallyLR implicit $w0

# End machine code for function f.

The combiner does something very fishy:

Try combining %5:_(s32) = G_SELECT %4:_(s1), %3:_, %2:_
10: GIM_SwitchOpcode(MIs[0], [19, 274), Default=6760, JumpTable...) // Got=142
3921: Begin try-block
3928: GIM_CheckSimplePredicate(Predicate=51)
3932: GIM_CheckCxxPredicate(MIs[0], Predicate=23)
3935: GIR_CustomAction(FnID=15)
Erasing: %5:_(s32) = G_SELECT %4:_(s1), %3:_, %2:_

Changing: %7:_(s1) = G_ICMP intpred(sgt), %5:_(s32), %6:_

Changed: %7:_(s1) = G_ICMP intpred(sgt), %3:_(s32), %6:_

3936: GIR_Done

When swapping the operands in select with the following %5 = select i1 false, i32 %3, i32 %4 that's %4 that's the first operand which is selected:

# *** IR Dump After IRTranslator (irtranslator) ***:
# Machine code for function f: IsSSA, TracksLiveness
Function Live Ins: $w0, $w1

bb.1 (%ir-block.2):
  liveins: $w0, $w1
  %0:_(s32) = COPY $w0
  %1:_(s32) = COPY $w1
  %4:_(s1) = G_CONSTANT i1 false
  %6:_(s32) = G_CONSTANT i32 0
  %9:_(s32) = G_CONSTANT i32 1
  %2:_(s32) = G_SUB %1:_, %0:_
  %3:_(s32) = nsw G_SUB %1:_, %0:_
  %5:_(s32) = G_SELECT %4:_(s1), %2:_, %3:_
  %7:_(s1) = G_ICMP intpred(sgt), %5:_(s32), %6:_
  %8:_(s32) = G_SELECT %7:_(s1), %6:_, %9:_
  $w0 = COPY %8:_(s32)
  RET_ReallyLR implicit $w0

# End machine code for function f.

[...]

Try combining %5:_(s32) = G_SELECT %4:_(s1), %2:_, %3:_
10: GIM_SwitchOpcode(MIs[0], [19, 274), Default=6760, JumpTable...) // Got=142
3921: Begin try-block
3928: GIM_CheckSimplePredicate(Predicate=51)
3932: GIM_CheckCxxPredicate(MIs[0], Predicate=23)
3935: GIR_CustomAction(FnID=15)
Erasing: %5:_(s32) = G_SELECT %4:_(s1), %2:_, %3:_

Changing: %7:_(s1) = G_ICMP intpred(sgt), %5:_(s32), %6:_

Changed: %7:_(s1) = G_ICMP intpred(sgt), %2:_(s32), %6:_

Now, if we remove nosw, we can see that the G_SELECT is changed before the erase:

Changing: %5:_(s32) = G_SELECT %4:_(s1), %3:_, %2:_

CSEInfo::Recording new MI %5:_(s32) = G_SELECT %4:_(s1), %3:_, %2:_
Changed: %5:_(s32) = G_SELECT %4:_(s1), %2:_, %2:_

CSEInfo::Recording new MI %5:_(s32) = G_SELECT %4:_(s1), %2:_, %2:_
1046: GIR_Done
v01dXYZ commented 3 months ago

I think I've got the reason. It seems the two subs are seen as equivalent (ie ~= select false, A, A) which makes the instruction match the select_same_val pattern in Combine.td. cf CodeGen/GlobalISel/CombinerHelper.cpp matchSelectSameVal and matchEqualDefs. Beneath it uses produceSameValue(...) -> MachineInstr::isIdentical. The code is at:

https://github.com/llvm/llvm-project/blob/b2c9f7d3188e41163574a83a835437955cf4b80f/llvm/lib/CodeGen/MachineInstr.cpp#L642

I don't know if it is a fault as marking one as nsw could also imply the other one as nsw. BUT swapping the two operands produce different outputs, so in this sense the two should not be seen as equivalent.

tschuett commented 3 months ago

From a brief scan of isDenticalTo, it looks as if it ignores flags.