[CIR][ThroughMLIR] Lower simple SwitchOp #1742
Open
+168,535
−44,645
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
bcardosolopes
left a comment
bcardosolopes
left a comment
There was a problem hiding this comment.
Overall looks good, mostly nits
lanza
force-pushed
the
main
branch
2 times, most recently
from
d2c4ab8 to
8f89224
Compare
Contributor
|
Rebase conflicts are now resolved. |
AdUhTkJm
force-pushed
the
cir-switch
branch
3 times, most recently
from
d8968f9 to
7dcc0ac
Compare
| break; | ||
| case CaseOpKind::Range: | ||
| case CaseOpKind::Anyof: | ||
| mlir::emitError(op.getLoc(), "not yet implemented"); |
Member
There was a problem hiding this comment.
Why not return here and in all other places?
This commit un-xfails some tests that were affected by upstream GEP changes. llvm#1497 The changes are likely introduced by inference of `inbounds` and `nuw` flags llvm/llvm-project@10f315d. It seems to allow LLVM to enable some constant foldings, whose effects include re-calculating the address using i8 and a single offset. (This change seems to arise from https://discourse.llvm.org/t/rfc-replacing-getelementptr-with-ptradd/68699/39)
Reapplying llvm@e66b670 which was reverted during rebase (after fixing some conflicts). Un-xfails the test `cc1.cir` (llvm#1497). --------- Co-authored-by: Nathan Lanza <[email protected]>
This commit attempts to un-xfail `globals-ref-globals` test (llvm#1497), which was causing a crash in addition to GEP changes. The crash was caused by incorrect offsets being calculated for global view indices. The original calculation did not take paddings into consideration, hence triggering a crash. This commit adds type alignment when calculating the offset, which will take care of paddings. Further modification to `globals-ref-globals` test includes fixing some struct names that got changed, as well as replacing the expects with constant folded GEP.
Previously, when emitting a global for a string literal, we were creating a GlobalOp, building a GlobalView attr for it, and looking up the global from the symbol associated with the attr. This change splits out the function that creates the global so that the global is returned directly and the GlobalView attribute is only created in the case where it is needed. This also updates the mechanism used for uniquing the global name used for the strings so that if different base names are used the uniquing numbers each base name separately. The mangling of the global used for strings is not implemented, but the uniquing was happening prior to the mangling. This change drops the uniquing below the placeholder for mangling.
This change corrects the alignment of store operations and fixes a related problem with calculation of member offsets (we weren't accounting for the alignment of the field whose offset we were calculating. Many tests are affected by this, but most just needed a wildcard match to ignore the explicit alignment which wasn't present before. In cases where I updated a check for a specific alignment value, I compared against classic codegen to verify that we are now producing the same alignment. Two new tests are added align-store.c and alignment.cpp. The second of these partially copies a test of the same name from clang/test/CodeGen. It's testing globals and isn't directly related to the code changes here, but we didn't seem to have a test for this. I put the store alignment tests in a different file because inconsistency between CIR and LLVM IR in placement of globals would have made a combined test difficult to follow. This addresses llvm#1204
Currently the ForOp handling ignores everything except load, store and
arithmetic in the step region. It does not detect whether the step and
induction variable has already been assigned, either.
That might result to wrong behaviour:
```cpp
// Ignores printf
for (int i = 0; i < n; i++, printf("\n"));
// Only increments once
for (int i = 0; i < n; i++, i++);
```
I choose to rewrite the detection and do an exact match of the
instruction sequence for `i++` and `i += n`. It doesn't seem easy to
detect a more general pattern without phi nodes.
The new test case is xfailed, because ForOp hits an unreachable when it
meets a non-canonicalized loop. We can implement that functionality
later.
Co-authored-by: Yue Huang <[email protected]>
…m#1638) This reverts commit 0c944f9. Reverting due to build failure reported in llvm#1635 (comment)
This adds an explicit alignment to load instructions where needed in order to make CIR load alignment consistent with classic codegen. As with aligned stores, I have updated tests that were failing with wildcard checks for cir.load, except where alignment was being specifically checked. Where tests failed because of changed alignment, I verified that the new alignment matches what is produced by classic codegen. The new test for proper alignment behavior is align-load.c.
…valuate unconditionally (llvm#1642) This came up during the review of llvm/llvm-project#138156 During codegen we check whether the LHS and RHS of the conditional operator are cheap enough to evaluate uncondionally. Unlike classic codegen we still emit `TernaryOp` instead of `SelectOp` and defer that optimization to cir-simplify. This patch changes codegen to directly emit `SelectOp` for `cond ? constant : constant` expressions.
From the [comment](llvm#1844 (comment)) on PR review llvm#1844, it seems like we're missing the flags for GEP. I'm opening the PR to add the flags. The first commit is just a prototype to gather opinions and reviews to see if I'm heading to the right direction with this.
Member
|
Tests are not passing yet :) |
…vm#1900) After llvm#1878, we introduced a dependency from the LoopOpInterface on BreakOp. While here add the BreakOp handling, which will be tests by a pass coming soon. Co-authored-by: Tommy McMichen <[email protected]>
Backport fixes in VisitAbstractConditionalOperator to handle OpaqueValueExpr from the upstream llvm/llvm-project#157331
Contributor
|
Just as a comment, I don't operate this account now - my internship has ended. Thanks for all your reviews in the past few months! |
Member
|
@AdUhTkJm you're welcome. Should I close this PR? |
…vm#1905) This PR adds to the implementation of `maybeEmitThunks` in `clang/lib/CIR/CodeGen/CIRGenVTables.cpp` . Newly declared/defined functions are ported from OG. Some missings are `Type::canLosslesslyBitCastTo` and `setDLLStorageClass`. No tests are added since the implementation is not finished yet.
Contributor
Author
I'm working on this PR. The test errors on Github look weird, tests passed on my local machine. I need some more time. |
Member
No rush, it was unclear from the convo, thanks for clarifying. You can probably commandeer this PR to the other gh user when you resume work! |
This PR adds support for the new `BlockAddressOp`, used for GCC labels as values. Support for indirect `goto` and `ConstantLValueEmitter::VisitAddrLabelExpr` will be added in a future PR.
This PR implements some missing blocks that allow us to effectively allow us to launch kernels from the host. All of the tests stated in this [commit](llvm@69f2099) are now resolved. I spent half a day figuring the following: I tried experiementing performing host compilation(`-fcuda-is-device`) with target triple: `nvptx64-nvidia-cuda` but was getting a module verification error that, to keep it simple looked like: `error: 'cir.call' op calling convention mismatch: expected ptx_kernel, but provided c`. I thought that was expected given that we're essentially using the device to compile on the host, which doesn't make a lot of sense. until I tried to replicate the same in OG and didn't really run into any problem in that regard. Are the calling conventions enforced in CIR much more strict as compared to OG? Or is that simply a bug from OG?
Fixes llvm#1818 - Implement createVecCompare, getCIRIntOrFloatBitWidth, getVectorFCmpIR helper for VecCmp op creation. - Add clang/test/CIR/CodeGen/builtin-fcmp-sse.c test. in OG, there is a sext from bool to int before casting to float vector since fcmp's result in llvm ir is boolean-like, while VecCmpOp in CIR returns int in the form of 0 or -1. There is also a boolean `shouldInvert` in CIR since CIR doesn't contain optimized unordered comparison, for example: OLE is the inverse predicate of UGT. So if we need UGT, we have to pass in OLE and `shouldInvert = true`
Backport using ArrayOf constraints from the upstream and the test file for invalid type info
This PR adds lowering of `BlockAddressOp`. It uses two maps, `blockInfoToTagOp` and `unresolvedBlockAddressOp`, to defer matching `mlir::LLVM::BlockAddressOp` to its corresponding `mlir::LLVM::BlockTagOp` in cases where the matching label has not yet been emitted. If the `BlockTagOp` is not emitted, a placeholder value `std::numeric_limits<uint32_t>::max()` is used, which will later be resolved in `resolveBlockAddressOp`. Support for indirect goto and label differences will be added in a future PR.
This PR adds supports for __builtin_ia32_cmpnltps/cmpnltpd. Depends on llvm#1893.
lanza
force-pushed
the
main
branch
2 times, most recently
from
8acaf96 to
58135ea
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
27 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This deals with fall-through by copying the body of the next
cir.caseto the previous case. This is needed becausescf.index_switchdoes not support falling through.