Bump LLVM #6404

girishpai · 2023-11-09T22:45:58Z

The upstream change to !llvm.ptr has triggered a lot of churn in the LLHD side of CIRCT. We currently don't rely on llhd-sim for anything in-tree. A few breakages remain in the interaction between the llhd-sim runtime and the generated code:

test/Dialect/LLHD/Simulator/sim_formats.mlir
test/Dialect/LLHD/Simulator/sim_process.mlir
test/Dialect/LLHD/Simulator/sim_reg.mlir
test/Dialect/LLHD/Simulator/sim_wait.mlir

These are now gated behind a REQUIRES: llhd-sim-fixed in order to unblock the LLVM bump and allow for them to be fixed in parallel. We might want to introduce a few intermediate ops in the LLHD dialect to reduce the pretty big abstraction gap between LLHD ops and LLVM. A lot of things can now go through the SCF dialect (and maybe memrefs?).

girishpai · 2023-11-09T22:48:31Z

@fabianschuiki @maerhart - needed your help based on @mikeurbach suggestion - getting the following compile errors on the latest bump.

 error: no matching function for call to 'mlir::LLVM::LLVMPointerType::get(mlir::Type)'
           LLVM::LLVMPointerType::get(adaptor.getInput().getType()), oneC,

In file included from /scratch/girishp/circt_clones/circt/llvm/mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.h:51,
                 from /scratch/girishp/circt_clones/circt/llvm/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrs.h:17,
/scratch/girishp/circt_clones/circt/lib/Conversion/HWToLLVM/HWToLLVM.cpp:170:55: error: 'class mlir::LLVM::LLVMPointerType' has no member named 'isOpaque'
     if (cast<LLVM::LLVMPointerType>(arrPtr.getType()).isOpaque())
                                                       ^~~~~~~~

fabianschuiki · 2023-11-09T22:58:33Z

I'll drop the isOpaque parts now that that seems to have migrated fully.

fabianschuiki · 2023-11-09T23:48:46Z

I've pushed my tweaks. I didn't catch everything yet. There's quite a bit of pointer stuff going on on the LLHD side -- I probably missed a few there.

girishpai · 2023-11-10T00:28:51Z

I've pushed my tweaks. I didn't catch everything yet. There's quite a bit of pointer stuff going on on the LLHD side -- I probably missed a few there.

Thanks @fabianschuiki looks like CI still failed - should we wait on @maerhart for the rest of the fixes?

girishpai · 2023-11-10T03:34:04Z

@fabianschuiki @maerhart continuing to see errors - unfortunately seems like lot of onion peeling. Could either of you look into this?

/scratch/girishp/circt_clones/circt/lib/Conversion/LLHDToLLVM/LLHDToLLVM.cpp:1424:59: error: no matching function for call to ‘mlir::LLVM::LLVMPointerType::get(mlir::Type&)’
           op->getLoc(), LLVM::LLVMPointerType::get(finalTy), sigDetail[0]);
                                                           ^
In file included from /scratch/girishp/circt_clones/circt/llvm/mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.h:51,
                 from /scratch/girishp/circt_clones/circt/llvm/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrs.h:17,
                 from /scratch/girishp/circt_clones/circt/llvm/mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h:18,
                 from /scratch/girishp/circt_clones/circt/lib/Conversion/LLHDToLLVM/LLHDToLLVM.cpp:30:
/scratch/girishp/circt_clones/circt/llvm/build/tools/mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.h.inc:104:26: note: candidate: ‘static mlir::LLVM::LLVMPointerType mlir::LLVM::LLVMPointerType::get(mlir::MLIRContext*, unsigned int)’
   static LLVMPointerType get(::mlir::MLIRContext *context, unsigned addressSpace = 0);
                          ^~~
/scratch/girishp/circt_clones/circt/llvm/build/tools/mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.h.inc:104:26: note:   no known conversion for argument 1 from ‘mlir::Type’ to ‘mlir::MLIRContext*’
/scratch/girishp/circt_clones/circt/lib/Conversion/LLHDToLLVM/LLHDToLLVM.cpp: In member function ‘virtual mlir::LogicalResult {anonymous}::DrvOpConversion::matchAndRewrite(mlir::Operation*, llvm::ArrayRef<mlir::Value>, mlir::ConversionPatternRewriter&) const’:
/scratch/girishp/circt_clones/circt/lib/Conversion/LLHDToLLVM/LLHDToLLVM.cpp:1478:78: error: no matching function for call to ‘mlir::LLVM::LLVMPointerType::get(mlir::Type)’
           LLVM::LLVMPointerType::get(typeConverter->convertType(underlyingTy));
                                                                              ^
In file included from /scratch/girishp/circt_clones/circt/llvm/mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.h:51,
                 from /scratch/girishp/circt_clones/circt/llvm/mlir/include/mlir/Dialect/LLVMIR/LLVMAttrs.h:17,
                 from /scratch/girishp/circt_clones/circt/llvm/mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h:18,
                 from /scratch/girishp/circt_clones/circt/lib/Conversion/LLHDToLLVM/LLHDToLLVM.cpp:30:
/scratch/girishp/circt_clones/circt/llvm/build/tools/mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.h.inc:104:26: note: candidate: ‘static mlir::LLVM::LLVMPointerType mlir::LLVM::LLVMPointerType::get(mlir::MLIRContext*, unsigned int)’
   static LLVMPointerType get(::mlir::MLIRContext *context, unsigned addressSpace = 0);
                          ^~~
/scratch/girishp/circt_clones/circt/llvm/build/tools/mlir/include/mlir/Dialect/LLVMIR/LLVMTypes.h.inc:104:26: note:   no known conversion for argument 1 from ‘mlir::Type’ to ‘mlir::MLIRContext*’
/scratch/girishp/circt_clones/circt/lib/Conversion/LLHDToLLVM/LLHDToLLVM.cpp:1526:55: error: no matching function for call to ‘mlir::LLVM::LLVMPointerType::get(mlir::Type&)’
         op->getLoc(), LLVM::LLVMPointerType::get(valTy), oneConst, 4);

fabianschuiki · 2023-11-13T22:24:39Z

Going to do another pass here to get us through the build.

fabianschuiki · 2023-11-13T23:36:40Z

Fixed all the compilation errors. Things now build properly and then fail in the tests. Most of them are probably just changes in MLIR text output and not actual breakages. Going to take a look at those next.

We currently don't rely on `llhd-sim` for anything in-tree. The upstream change to `!llvm.ptr` has triggered a lot of churn in the LLHD side of CIRCT. A few breakages remain in the interaction between the llhd-sim runtime and the generated code. These are now gated behind a `REQUIRES: llhd-sim-fixed` in order to unblock the LLVM bump and allow for them to be fixed in parallel.

fabianschuiki

I've fixed the remaining breakages in the LLHD, HW, and Arc conversions to LLVM. There are still a few runtime breakages in llhd-sim itself, but I've disabled the corresponding tests for now to unblock this LLVM bump. Some of those breakages are likely very subtle and will take some time to track down.

@girishpai You probably want to do a "Squash and Merge" to compress all the small fix commits into one big bump commit, and preserve the PR message as commit message to leave a few breadcrumbs regarding the disabled llhd-sim tests.

girishpai · 2023-11-15T00:48:47Z

Thanks @fabianschuiki - I am seeing some failures in short integration tests - related to Python bindnigs

Failed Tests (2):
  CIRCT :: Bindings/Python/dialects/hw.py
  CIRCT :: Bindings/Python/dialects/om.py


Testing Time: 16.01s

Total Discovered Tests: 89
  Unsupported:  3 (3.37%)
  Passed     : 84 (94.38%)
  Failed     :  2 (2.25%)
FAILED: tools/circt/integration_test/CMakeFiles/check-circt-integration /__w/circt/circt/build/tools/circt/integration_test/CMakeFiles/check-circt-integration 
cd /__w/circt/circt/build/tools/circt/integration_test && /usr/bin/python3.9 /__w/circt/circt/build/./bin/llvm-lit -v --show-unsupported /__w/circt/circt/build/tools/circt/integration_test

@mikeurbach any chance its related to your area of expertise?

mikeurbach · 2023-11-15T00:51:57Z

Let me take a look at those failures.

mikeurbach · 2023-11-15T02:12:55Z

@makslevental do you have any idea about this Python issue? What I'm observing is with the latest LLVM update, the automatic downcasting is not working. I'm wondering if you have any idea what might be wrong since you were poking around in here for the value casters since the last time CIRCT updated LLVM.

For example, one of the errors is here:

circt/integration_test/Bindings/Python/dialects/om.py

Line 223 in c65a4af

if hasattr(cls, "sym_name") and cls.sym_name.value == "Paths"

In this case, cls.sym_name is expected to be a StringAttr, but now it is just Attribute. I've tried calling cls.sym_name.maybe_downcast(), and this returns Attribute, if that helps pinpoint the issue at all.

Another example is here:

circt/integration_test/Bindings/Python/dialects/hw.py

Lines 129 to 130 in c65a4af

    
           # CHECK-NEXT:  [IntegerType(i2), IntegerType(i32), Type(!hw.inout<i32>)] 
        
           print(module_type.input_types)

ModuleType is our own type, and its input_types returns a list of MlirType:

circt/lib/Bindings/Python/HWModule.cpp

Line 75 in c65a4af

.def_property_readonly(

Previously, the IntegerTypes would automatically be downcast, which our test was checking. Now they are coming back as just Type.

I looked through recent commits to MLIR, and I don't see anything that was obviously updated here, so I'm curious if you have any ideas.

makslevental · 2023-11-15T02:24:26Z

I suspect it's this change that's malfunctioning here for you llvm/llvm-project@7c85086#diff-9f49e93375e3ede73d748a7b5f9809da29e96c7f62674cd8b9eaf50181bd17ed, since all of these types and attributes are mlir_sub... things. I can't figure it out just by looking at it (because you guys don't have getTypeIDFunction functions?) but let me clone/debug.

mikeurbach · 2023-11-15T02:33:34Z

Thanks for taking a look, but I think I've got it. It's actually related to this: llvm/llvm-project@5192e29#diff-1f295ae31d665f9a778ea210c0816a22c621c34d54bf5b83b21686bf0432a918R126.

With that change, loadDialectModule now returns a boolean if it successfully loaded the dialect or had already loaded it, and lookupTypeCaster and friends only proceed if it returned true.

What do the Attribute and Type I mentioned previously have in common? They're builtin. There is nothing to load, so within loadDialectModule, loaded.is_none() is always true, that method returns false, and lookupTypeCaster exits early.

I'm pretty sure this is it, with a small patch the automatic downcasting is working again. I'll make a small test case to show the issue and send a PR.

mikeurbach · 2023-11-15T02:34:36Z

@girishpai I'm not sure if there's any way to work around this in CIRCT, so let's hold this PR. If I'm on the right track, we might want to wait until the fix is in MLIR and update again.

makslevental · 2023-11-15T02:39:09Z

What do the Attribute and Type I mentioned previously have in common? They're builtin. There is nothing to load, so

What do you mean by "builtin"? I think you mean mlir_sub...?

that method returns false, and lookupTypeCaster exits early.

If you don't have a dialect (which I know you do) none of the typeCasters would be usable for you, so the bail is warranted.

I'm not sure if there's any way to work around this in CIRCT, so let's hold this PR. If I'm on the right track, we might want to wait until the fix is in MLIR and update again.

There is a means and it's described in the same PR but it's only for unregistered dialects so I don't know how it could be something that worked for you previously.

mikeurbach · 2023-11-15T03:25:12Z

@makslevental helped me figure this out. The issue I described above was the root cause, but the fix is actually simpler: make sure the builtin dialect actually gets loaded. This is something we can control in CIRCT, and the latest commit fixes this. We're discussing that we probably shouldn't have to do this manually, but for now we can proceed.

mikeurbach · 2023-11-15T03:32:55Z

I'm going to go ahead and merge this. No sense in waiting.

mikeurbach · 2023-11-15T03:41:58Z

@teqdruid you might not hit the same issue in PyCDE, but I just remembered that you have your own CMake setup there. I didn't update it, but if there is any problem, the short term solution is to apply the last commit on this PR. The longer term solution would be the linked PR.

girishpai · 2023-11-15T15:12:25Z

Thanks a lot @mikeurbach @fabianschuiki @maerhart for taking care of all the issues promptly!!

Bump LLVM

8aaf57f

girishpai requested review from fabianschuiki, maerhart and mikeurbach November 9, 2023 22:49

Address some of the LLVM pointer type changes

1eaa6b7

Fix remaining build breakages; test failures remain

dc39572

fabianschuiki added 11 commits November 13, 2023 15:52

Fix ArcToLLVM test breakages

f0ceb0e

Fix HWToLLVM conversion

76d55f9

Fix LLHD convert_entity and convert_simple tests

08e5929

Fix convert_signals test

ed36a0d

Fix convert_memory test

76d8e00

Fix signal-init test

a7b5553

Fix convert_wait test

b4ee64e

Fix convert_extract test

67331ca

Fix convert_process_persistence test

422bf52

Add test for llhd.halt

b824206

fabianschuiki approved these changes Nov 15, 2023

View reviewed changes

Include the builtin dialect.

ed2bc9d

mikeurbach merged commit 02f0bff into main Nov 15, 2023

mikeurbach deleted the girish-llvm-bump-1d1fede49 branch November 15, 2023 03:35

makslevental mentioned this pull request Nov 15, 2023

[mlir][python] remove eager loading of dialect module (for type and value casting) llvm/llvm-project#72338

Merged

mikeurbach mentioned this pull request Nov 20, 2023

[PyCDE] Fixing everything unexpectedly broken #6430

Merged

Bump LLVM #6404

Bump LLVM #6404

Uh oh!

Conversation

girishpai commented Nov 9, 2023 • edited by fabianschuiki Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

girishpai commented Nov 9, 2023

Uh oh!

fabianschuiki commented Nov 9, 2023

Uh oh!

fabianschuiki commented Nov 9, 2023

Uh oh!

girishpai commented Nov 10, 2023

Uh oh!

girishpai commented Nov 10, 2023

Uh oh!

fabianschuiki commented Nov 13, 2023

Uh oh!

fabianschuiki commented Nov 13, 2023

Uh oh!

fabianschuiki left a comment

Choose a reason for hiding this comment

Uh oh!

girishpai commented Nov 15, 2023

Uh oh!

mikeurbach commented Nov 15, 2023

Uh oh!

mikeurbach commented Nov 15, 2023

Uh oh!

makslevental commented Nov 15, 2023

Uh oh!

mikeurbach commented Nov 15, 2023

Uh oh!

mikeurbach commented Nov 15, 2023

Uh oh!

makslevental commented Nov 15, 2023

Uh oh!

mikeurbach commented Nov 15, 2023

Uh oh!

mikeurbach commented Nov 15, 2023

Uh oh!

mikeurbach commented Nov 15, 2023

Uh oh!

girishpai commented Nov 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

girishpai commented Nov 9, 2023 •

edited by fabianschuiki

Loading