-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
InstCombine Transform Broken for set[lg]t (cast X to X++), CI #826
Comments
assigned to @lattner |
This testcase may work better: bool %nada(sbyte %X) { Otherwise, instcombine will just delete all of the code. :) In this case, it should be optimized to: bool %nada(sbyte %X) { -Chris |
Potential Patch |
Test Case |
The "Potential Patch" produces the following xform from the "Test Case". I'd bool %lt_signed_to_large_unsigned(sbyte %SB) { |
Okay, the new patch now delivers: bool %lt_signed_to_large_unsigned(sbyte %SB) { bool %lt_signed_to_large_signed(sbyte %SB) { bool %lt_signed_to_large_negative(sbyte %SB) { bool %lt_signed_to_small_unsigned(sbyte %SB) { bool %lt_signed_to_small_signed(sbyte %SB) { bool %lt_signed_to_small_negative(sbyte %SB) { bool %lt_unsigned_to_large_unsigned(ubyte %SB) { bool %lt_unsigned_to_large_signed(ubyte %SB) { bool %lt_unsigned_to_large_negative(ubyte %SB) { bool %lt_unsigned_to_small_unsigned(ubyte %SB) { bool %lt_unsigned_to_small_signed(ubyte %SB) { bool %lt_unsigned_to_small_negative(ubyte %SB) { bool %gt_signed_to_large_unsigned(sbyte %SB) { bool %gt_signed_to_large_signed(sbyte %SB) { bool %gt_signed_to_large_negative(sbyte %SB) { bool %gt_signed_to_small_unsigned(sbyte %SB) { bool %gt_signed_to_small_signed(sbyte %SB) { bool %gt_signed_to_small_negative(sbyte %SB) { bool %gt_unsigned_to_large_unsigned(ubyte %SB) { bool %gt_unsigned_to_large_signed(ubyte %SB) { bool %gt_unsigned_to_large_negative(ubyte %SB) { bool %gt_unsigned_to_small_unsigned(ubyte %SB) { bool %gt_unsigned_to_small_signed(ubyte %SB) { bool %gt_unsigned_to_small_negative(ubyte %SB) { |
Third Potential Patch |
The last patch is currently awaiting results of nightly testing. It will be |
Resolution Patch Is Here: http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041122/021408.html |
Part of this patch was disabled here: It miscompiles the testcase in the commit. -Chris |
This transform was reimplemented and reenabled here: -Chris |
I just want to add a short note that the code contains several shifts in the Maybe it would be a good idea to implement some "uint64_t getHighBit(unsigned |
Excellent point, I'll audit these. Thanks! -Chris |
Yup, there were bugs. Fixed here: Thanks! -Chris |
We were previously printing the type alias for `struct S` as `!ty_22S22` instead of just `!ty_S`. This was because our alias computation for a StructType did the following: os << "ty_" << structType.getName() `structType.getName()` is a `StringAttr`, and writing a `StringAttr` to an output stream adds double quotes around the actual string [1][2]. These double quotes then get hex-encoded as 22 [3]. We can fix this by writing the actual string value instead. Aliases that would end in a number will now receive a trailing underscore because of MLIR's alias sanitization not allowing a trailing digit [4] (which ironically didn't kick in even though our aliases were always previously ending with a number, which might be a bug in the sanitization logic). Aliases containing other special characters (e.g. `::`) will still be escaped as before. In other words: ``` struct S {}; // before: !ty_22S22 = ... // after: !ty_S = ... struct S1 {}; // before: !ty_22S122 = ... // after: !ty_S1_ = ... struct std::string {}; // before: !ty_22std3A3Astring22 // after: !ty_std3A3Astring ``` I'm not a big fan of the trailing underscore special-case, but I also don't want to touch core MLIR logic, and I think the end result is still nicer than before. The tests were mechanically updated with the following command run inside `clang/test/CIR`, and the same commands can be run to update the tests for any in-flight patches. (These are for GNU sed; for macOS change the `-i` to `-i ''`.) find . -type f | xargs sed -i -E -e 's/ty_22([A-Za-z0-9_$]+[0-9])22/ty_\1_/g' -e 's/ty_22([A-Za-z0-9_$]+)22/ty_\1/g' clang/test/CIR/CodeGen/stmtexpr-init.c needed an additional minor fix to swap the expected order of two type aliases in the CIR output. clang/test/CIR/CodeGen/coro-task.cpp needed some surgery because it was searching for `22` to find the end of a type alias; I changed it to search for the actual alias instead. If you run into merge conflicts after this change, you can auto-resolve them via smeenai/clangir@715f061 [1] https://github.com/llvm/llvm-project/blob/b3d2d5039b9b8aa10a86c593387f200b15c02aef/mlir/lib/IR/AsmPrinter.cpp#L2295 [2] https://github.com/llvm/llvm-project/blob/b3d2d5039b9b8aa10a86c593387f200b15c02aef/mlir/lib/IR/AsmPrinter.cpp#L2763 [3] https://github.com/llvm/llvm-project/blob/b3d2d5039b9b8aa10a86c593387f200b15c02aef/mlir/lib/IR/AsmPrinter.cpp#L1014 [4] https://github.com/llvm/llvm-project/blob/b3d2d5039b9b8aa10a86c593387f200b15c02aef/mlir/lib/IR/AsmPrinter.cpp#L1154
We were previously printing the type alias for `struct S` as `!ty_22S22` instead of just `!ty_S`. This was because our alias computation for a StructType did the following: os << "ty_" << structType.getName() `structType.getName()` is a `StringAttr`, and writing a `StringAttr` to an output stream adds double quotes around the actual string [1][2]. These double quotes then get hex-encoded as 22 [3]. We can fix this by writing the actual string value instead. Aliases that would end in a number will now receive a trailing underscore because of MLIR's alias sanitization not allowing a trailing digit [4] (which ironically didn't kick in even though our aliases were always previously ending with a number, which might be a bug in the sanitization logic). Aliases containing other special characters (e.g. `::`) will still be escaped as before. In other words: ``` struct S {}; // before: !ty_22S22 = ... // after: !ty_S = ... struct S1 {}; // before: !ty_22S122 = ... // after: !ty_S1_ = ... struct std::string {}; // before: !ty_22std3A3Astring22 // after: !ty_std3A3Astring ``` I'm not a big fan of the trailing underscore special-case, but I also don't want to touch core MLIR logic, and I think the end result is still nicer than before. The tests were mechanically updated with the following command run inside `clang/test/CIR`, and the same commands can be run to update the tests for any in-flight patches. (These are for GNU sed; for macOS change the `-i` to `-i ''`.) find . -type f | xargs sed -i -E -e 's/ty_22([A-Za-z0-9_$]+[0-9])22/ty_\1_/g' -e 's/ty_22([A-Za-z0-9_$]+)22/ty_\1/g' clang/test/CIR/CodeGen/stmtexpr-init.c needed an additional minor fix to swap the expected order of two type aliases in the CIR output. clang/test/CIR/CodeGen/coro-task.cpp needed some surgery because it was searching for `22` to find the end of a type alias; I changed it to search for the actual alias instead. If you run into merge conflicts after this change, you can auto-resolve them via smeenai/clangir@715f061 [1] https://github.com/llvm/llvm-project/blob/b3d2d5039b9b8aa10a86c593387f200b15c02aef/mlir/lib/IR/AsmPrinter.cpp#L2295 [2] https://github.com/llvm/llvm-project/blob/b3d2d5039b9b8aa10a86c593387f200b15c02aef/mlir/lib/IR/AsmPrinter.cpp#L2763 [3] https://github.com/llvm/llvm-project/blob/b3d2d5039b9b8aa10a86c593387f200b15c02aef/mlir/lib/IR/AsmPrinter.cpp#L1014 [4] https://github.com/llvm/llvm-project/blob/b3d2d5039b9b8aa10a86c593387f200b15c02aef/mlir/lib/IR/AsmPrinter.cpp#L1154
Extended Description
Currently, the instruction combiner is broken when it tries to eliminate a setcc
with a cast and a constant as an argument under a number of constraints:
The code in question is the switch case starting at InstructionCombining.cpp:2021.
Currently, the optimization has been disabled (leading to performance
degradation of generated code). The disablement was provided by this patch:
http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041101/020140.html
This bug aims to fix the pessimization by correctly re-enabling the optimization.
The text was updated successfully, but these errors were encountered: