Skip to content

Conversation

@raikonenfnu
Copy link
Collaborator

@raikonenfnu raikonenfnu commented Aug 16, 2024

Bump LLVM upstream while reverting llvm/llvm-project#104630 due to issues which will be detailed below.

Potential fixes:
llvm/llvm-project#104630 for issue no.1
llvm/llvm-project#104648 for issue no.2

llvm/llvm-project#104630 breaks 2 things:

1.vm.call + emitc to c code generation.

error: null operand found
    vm.check.eq %ref_dno, %res, "_v_r()=NULL" : !vm.ref<?>
%11 = "emitc.call_opaque"(%8, <<NULL VALUE>>) <{callee = "vm_cmp_eq_ref"}> : (!emitc.ptr<!emitc.opaque<"iree_vm_ref_t">>, <<NULL TYPE>>) -> i32
  1. memref_util_test failure due to populateMemrefUtilPatterns not properly legalizing builtin.unrealized_conversion_cast, even though we expected builtin.unrealized_conversion_cast.
memref_util_test.mlir:1:79: error: failed to legalize unresolved materialization from ('i32') to 'i16' that remained live after conversion
util.func @load_store_i16(%buffer: memref<?xi16>, %idx0: index, %idx1: index, %value: i16) -> i16 {
                                                                              ^
memref_util_test.mlir:1:79: note: see current operation: %0 = "builtin.unrealized_conversion_cast"(%arg3) : (i32) -> i16
memref_util_test.mlir:7:3: note: see existing live user here: "util.buffer.store"(%0, %arg0, %4, %6, %5) : (i16, !util.buffer, index, index, index) -> ()
  memref.store %value, %buffer[%idx0] : memref<?xi16>
  ^
memref_util_test.mlir:0:0: error: conversion to util failed
memref_util_test.mlir:0:0: note: see current operation:
"builtin.module"() ({
  "util.func"() <{function_type = (memref<?xi16>, index, index, i16) -> i16, sym_name = "load_store_i16", tied_operands = [-1 : index]}> ({
  ^bb0(%arg0: memref<?xi16>, %arg1: index, %arg2: index, %arg3: i16):
    "memref.store"(%arg3, %arg0, %arg1) : (i16, memref<?xi16>, index) -> ()
    %0 = "memref.load"(%arg0, %arg2) : (memref<?xi16>, index) -> i16
    "util.return"(%0) : (i16) -> ()
  }) : () -> ()
}) : () -> ()

Signed-off-by: Stanley Winata <[email protected]>
Signed-off-by: Stanley Winata <[email protected]>
@MaheshRavishankar
Copy link
Collaborator

Do you want to wait for the upstream commit to be reviewed and landed.

@raikonenfnu
Copy link
Collaborator Author

raikonenfnu commented Aug 16, 2024

Currently llvm/llvm-project#104630 breaks 2 things:

1.vm.call + emitc to c code generation.

error: null operand found
    vm.check.eq %ref_dno, %res, "_v_r()=NULL" : !vm.ref<?>
%11 = "emitc.call_opaque"(%8, <<NULL VALUE>>) <{callee = "vm_cmp_eq_ref"}> : (!emitc.ptr<!emitc.opaque<"iree_vm_ref_t">>, <<NULL TYPE>>) -> i32
  1. memref_util_test failure due to populateMemrefUtilPatterns not properly legalizing builtin.unrealized_conversion_cast, even though we expected builtin.unrealized_conversion_cast.
memref_util_test.mlir:1:79: error: failed to legalize unresolved materialization from ('i32') to 'i16' that remained live after conversion
util.func @load_store_i16(%buffer: memref<?xi16>, %idx0: index, %idx1: index, %value: i16) -> i16 {
                                                                              ^
memref_util_test.mlir:1:79: note: see current operation: %0 = "builtin.unrealized_conversion_cast"(%arg3) : (i32) -> i16
memref_util_test.mlir:7:3: note: see existing live user here: "util.buffer.store"(%0, %arg0, %4, %6, %5) : (i16, !util.buffer, index, index, index) -> ()
  memref.store %value, %buffer[%idx0] : memref<?xi16>
  ^
memref_util_test.mlir:0:0: error: conversion to util failed
memref_util_test.mlir:0:0: note: see current operation:
"builtin.module"() ({
  "util.func"() <{function_type = (memref<?xi16>, index, index, i16) -> i16, sym_name = "load_store_i16", tied_operands = [-1 : index]}> ({
  ^bb0(%arg0: memref<?xi16>, %arg1: index, %arg2: index, %arg3: i16):
    "memref.store"(%arg3, %arg0, %arg1) : (i16, memref<?xi16>, index) -> ()
    %0 = "memref.load"(%arg0, %arg2) : (memref<?xi16>, index) -> i16
    "util.return"(%0) : (i16) -> ()
  }) : () -> ()
}) : () -> ()

llvm/llvm-project#104630 fixes issue no.1, however issue no.2 is still unsolved. I tried doing fixing it inside IREE by legalizing UnrealizedConversionCast, but seems like something a little more complex under the hood is going on.

@raikonenfnu
Copy link
Collaborator Author

raikonenfnu commented Aug 16, 2024

Do you want to wait for the upstream commit to be reviewed and landed.

I'd like to land this for now since I don't want us to get too behind on the llvm upstreaming. If we do this, I will be sure to "ramp up" the next person on why we need this revert and when we can bring it back.

But open to otherwise. What do you think @MaheshRavishankar? :)

@MaheshRavishankar
Copy link
Collaborator

Sounds good. stamped

@hanhanW
Copy link
Contributor

hanhanW commented Aug 17, 2024

Please put the reverted commits to the PR description. It is very helpful for tracking the local revert commits and cherry-picks.

@raikonenfnu
Copy link
Collaborator Author

Please put the reverted commits to the PR description. It is very helpful for tracking the local revert commits and cherry-picks.

Done :)

@raikonenfnu raikonenfnu merged commit a884b93 into iree-org:main Aug 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants