Skip to content

Commit 855a0c1

Browse files
damyanpCopilot
andcommitted
Fix #8598: don't emit *.with.overflow intrinsics for DXIL
InstCombine folds the "widened multiply/add compared against a max value" overflow-check idioms into llvm.umul.with.overflow / llvm.sadd.with.overflow. These intrinsics (and the extractvalue of their result struct) are not legal in DXIL, so shaders using them failed validation once optimizations were on. Guard ProcessUMulZExtIdiom and ProcessUGT_ADDCST_ADD to bail out for the DXIL target, mirroring the existing isDXIL() guard on bswap matching. The plain multiply/add and compare are kept, which is what DXIL wants anyway. Adds .ll regression tests for both idioms plus an end-to-end HLSL test built from the issue repro. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 5853990 commit 855a0c1

4 files changed

Lines changed: 79 additions & 0 deletions

File tree

lib/Transforms/InstCombine/InstCombineCompares.cpp

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
#include "InstCombineInternal.h"
1515
#include "llvm/ADT/APSInt.h"
1616
#include "llvm/ADT/Statistic.h"
17+
#include "llvm/ADT/Triple.h" // HLSL Change
1718
#include "llvm/Analysis/ConstantFolding.h"
1819
#include "llvm/Analysis/InstructionSimplify.h"
1920
#include "llvm/Analysis/MemoryBuiltins.h"
@@ -2053,6 +2054,10 @@ Instruction *InstCombiner::visitICmpInstWithCastAndCast(ICmpInst &ICI) {
20532054
static Instruction *ProcessUGT_ADDCST_ADD(ICmpInst &I, Value *A, Value *B,
20542055
ConstantInt *CI2, ConstantInt *CI1,
20552056
InstCombiner &IC) {
2057+
// HLSL Change begin - DXIL has no sadd.with.overflow intrinsic.
2058+
if (Triple(I.getModule()->getTargetTriple()).isDXIL())
2059+
return nullptr;
2060+
// HLSL Change end
20562061
// The transformation we're trying to do here is to transform this into an
20572062
// llvm.sadd.with.overflow. To do this, we have to replace the original add
20582063
// with a narrower add, and discard the add-with-constant that is part of the
@@ -2245,6 +2250,10 @@ bool InstCombiner::OptimizeOverflowCheck(OverflowCheckFlavor OCF, Value *LHS,
22452250
/// replacement required.
22462251
static Instruction *ProcessUMulZExtIdiom(ICmpInst &I, Value *MulVal,
22472252
Value *OtherVal, InstCombiner &IC) {
2253+
// HLSL Change begin - DXIL has no umul.with.overflow intrinsic.
2254+
if (Triple(I.getModule()->getTargetTriple()).isDXIL())
2255+
return nullptr;
2256+
// HLSL Change end
22482257
// Don't bother doing this transformation for pointers, don't do it for
22492258
// vectors.
22502259
if (!isa<IntegerType>(MulVal->getType()))
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
; RUN: %dxopt %s -instcombine -S | FileCheck %s
2+
3+
; Regression test for https://github.com/microsoft/DirectXShaderCompiler/issues/8598
4+
;
5+
; InstCombine used to fold this signed add overflow-check idiom into
6+
; llvm.sadd.with.overflow, which is not a legal DXIL intrinsic. For DXIL the
7+
; add/compare must be left alone.
8+
9+
target datalayout = "e-m:e-p:32:32-i1:32-i8:32-i16:32-i32:32-i64:64-f16:32-f32:32-f64:64-n8:16:32:64"
10+
target triple = "dxil-ms-dx"
11+
12+
; CHECK-LABEL: define i1 @test_sadd
13+
; CHECK-NOT: sadd.with.overflow
14+
; CHECK-NOT: extractvalue
15+
; CHECK: add {{(nsw )?}}i32
16+
; CHECK: icmp ugt i32
17+
define i1 @test_sadd(i8 %a8, i8 %b8) {
18+
%a = sext i8 %a8 to i32
19+
%b = sext i8 %b8 to i32
20+
%add = add i32 %a, %b
21+
%addcst = add i32 %add, 128
22+
%cmp = icmp ugt i32 %addcst, 255
23+
ret i1 %cmp
24+
}
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
; RUN: %dxopt %s -instcombine -S | FileCheck %s
2+
3+
; Regression test for https://github.com/microsoft/DirectXShaderCompiler/issues/8598
4+
;
5+
; InstCombine used to fold the "widened multiply compared against UINT_MAX"
6+
; idiom into llvm.umul.with.overflow, which is not a legal DXIL intrinsic and
7+
; failed validation. For DXIL the multiply/compare must be left alone.
8+
9+
target datalayout = "e-m:e-p:32:32-i1:32-i8:32-i16:32-i32:32-i64:64-f16:32-f32:32-f64:64-n8:16:32:64"
10+
target triple = "dxil-ms-dx"
11+
12+
; CHECK-LABEL: define i1 @test
13+
; CHECK-NOT: umul.with.overflow
14+
; CHECK-NOT: extractvalue
15+
; CHECK: mul {{(nuw )?}}i64 %xw, %yw
16+
; CHECK: icmp ugt i64
17+
define i1 @test(i32 %x, i32 %y) {
18+
%xw = zext i32 %x to i64
19+
%yw = zext i32 %y to i64
20+
%mul = mul i64 %xw, %yw
21+
%cmp = icmp ugt i64 %mul, 4294967295
22+
ret i1 %cmp
23+
}
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
// RUN: %dxc -E main -T cs_6_0 %s | FileCheck %s
2+
3+
// Regression test for https://github.com/microsoft/DirectXShaderCompiler/issues/8598
4+
//
5+
// The 32x32->64 bit multiply overflow check below used to be folded by
6+
// InstCombine into llvm.umul.with.overflow.i32, an intrinsic that is not legal
7+
// in DXIL and failed validation with optimizations enabled. Make sure it
8+
// compiles and validates, keeping a plain 64-bit multiply.
9+
10+
// CHECK-NOT: umul.with.overflow
11+
// CHECK: mul nuw i64
12+
13+
RWStructuredBuffer<uint> buf : register(u0);
14+
15+
[numthreads(1, 1, 1)]
16+
void main()
17+
{
18+
uint x = buf[0];
19+
uint y = buf[1];
20+
if (((uint64_t)x) * ((uint64_t)y) > 0xffffffffull) {
21+
buf[2] = 1;
22+
}
23+
}

0 commit comments

Comments
 (0)