nanoMIPS: Optimize 64bit comparisons when -Os #9
Conversation
This file contains 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
Comparison of two 64-bit data can be realized using three nanomips conditional branch instructions. To accomplish it, we're introducing custom lowering for the BRCOND node. This situation must be recognized as valid when we're analyzing the branch and when we're printing the assembler. Therefore, additional checks have been added for the MipsInstrInfo::analyzeBranch method in the case of nanoMIPS architecture. Also, there we set a LabelMustBeEmitted flag for the If.then block. We're trying to optimize when -Os is turned on.
milica-lazarevic
force-pushed
the
opt-brsc
branch
from
694a2d7 to
3a00d68
Compare
There was a problem hiding this comment.
Well, the patch looks alright, but I am not sure about pulling it in. Is there is a specific use-case or standard benchmark which would benefit from smaller 64-bit comparisons? Perhaps if the change were made with an eye to mips32R6 and also happened to help nanoMIPS, we might consider it differently. As it stands, I am not convinced that the increased complexity is justified by 8-byte occasional gain.
I'll get the numbers on a benchmark to see if it is justified and be back. Thanks for the review! |
cme
pushed a commit
that referenced
this pull request
Sep 18, 2023
For the following program,
$ cat t.c
struct t {
int (__attribute__((btf_type_tag("rcu"))) *f)();
int a;
};
int foo(struct t *arg) {
return arg->a;
}
Compiling with 'clang -g -O2 -S t.c' will cause a failure like below:
clang: /home/yhs/work/llvm-project/clang/lib/Sema/SemaType.cpp:6391: void {anonymous}::DeclaratorLocFiller::VisitParenTypeLoc(clang::ParenTypeLoc):
Assertion `Chunk.Kind == DeclaratorChunk::Paren' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script.
Stack dump:
......
#5 0x00007f89e4280ea5 abort (/lib64/libc.so.6+0x21ea5)
#6 0x00007f89e4280d79 _nl_load_domain.cold.0 (/lib64/libc.so.6+0x21d79)
#7 0x00007f89e42a6456 (/lib64/libc.so.6+0x47456)
#8 0x00000000045c2596 GetTypeSourceInfoForDeclarator((anonymous namespace)::TypeProcessingState&, clang::QualType, clang::TypeSourceInfo*) SemaType.cpp:0:0
#9 0x00000000045ccfa5 GetFullTypeForDeclarator((anonymous namespace)::TypeProcessingState&, clang::QualType, clang::TypeSourceInfo*) SemaType.cpp:0:0
......
The reason of the failure is due to the mismatch of TypeLoc and D.getTypeObject().Kind. For example,
the TypeLoc is
BTFTagAttributedType 0x88614e0 'int btf_type_tag(rcu)()' sugar
|-ParenType 0x8861480 'int ()' sugar
| `-FunctionNoProtoType 0x8861450 'int ()' cdecl
| `-BuiltinType 0x87fd500 'int'
while corresponding D.getTypeObject().Kind points to DeclaratorChunk::Paren, and
this will cause later assertion.
To fix the issue, similar to AttributedTypeLoc, let us skip BTFTagAttributedTypeLoc in
GetTypeSourceInfoForDeclarator().
Differential Revision: https://reviews.llvm.org/D136807
cme
pushed a commit
that referenced
this pull request
Sep 18, 2023
Change https://reviews.llvm.org/D140059 exposed the following crash in Z3Solver, where bit widths were not checked consistently with that change. This change makes the check consistent, and fixes the crash. ``` clang: <root>/llvm/include/llvm/ADT/APSInt.h:99: int64_t llvm::APSInt::getExtValue() const: Assertion `isRepresentableByInt64() && "Too many bits for int64_t"' failed. ... Stack dump: 0. Program arguments: clang -cc1 -internal-isystem <root>/lib/clang/16/include -nostdsysteminc -analyze -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-config crosscheck-with-z3=true -verify reproducer.c #0 0x00000000045b3476 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) <root>/llvm/lib/Support/Unix/Signals.inc:567:22 #1 0x00000000045b3862 PrintStackTraceSignalHandler(void*) <root>/llvm/lib/Support/Unix/Signals.inc:641:1 #2 0x00000000045b14a5 llvm::sys::RunSignalHandlers() <root>/llvm/lib/Support/Signals.cpp:104:20 #3 0x00000000045b2eb4 SignalHandler(int) <root>/llvm/lib/Support/Unix/Signals.inc:412:1 ... #9 0x0000000004be2eb3 llvm::APSInt::getExtValue() const <root>/llvm/include/llvm/ADT/APSInt.h:99:5 <root>/llvm/lib/Support/Z3Solver.cpp:740:53 clang::ASTContext&, clang::ento::SymExpr const*, llvm::APSInt const&, llvm::APSInt const&, bool) <root>/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h:552:61 ``` Reviewed By: steakhal Differential Revision: https://reviews.llvm.org/D142627 (cherry picked from commit f027dd5)
|
@milica-lazarevic what is the status of this? Can we port it to LLVM 16? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Comparison of two 64-bit data can be realized using three nanomips conditional branch instructions. To accomplish it, we're introducing custom lowering for the BRCOND node. This situation must be recognized as valid when we're analyzing the branch and when we're printing the assembler. Therefore, additional checks have been added for the MipsInstrInfo::analyzeBranch method in the case of nanoMIPS architecture. Also, there we set a LabelMustBeEmitted flag for the If.then block. We're trying to optimize when -Os is turned on.