Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add MC layer #1

Merged
merged 4 commits into from
Oct 17, 2018
Merged

Add MC layer #1

merged 4 commits into from
Oct 17, 2018

Conversation

mskvortsov
Copy link
Contributor

This change implements the AsmParser, CodeEmitter, Disassembler, AsmBackend
components. Also, the target description is updated with more instructions.

Please, take a look.

@mskvortsov
[MSP430] Add MC layer
599be70 
This change implements the AsmParser, CodeEmitter, Disassembler, AsmBackend
components.  Also, the target description is updated with more instructions.
asl
asl reviewed Oct 16, 2018
View reviewed changes
src/llvm/lib/Target/MSP430/MSP430ISelLowering.cpp
case MSP430ISD::SHL: return "MSP430ISD::SHL";
case MSP430ISD::SRA: return "MSP430ISD::SRA";
case MSP430ISD::SRL: return "MSP430ISD::SRL";
case MSP430ISD::DADD: return "MSP430ISD::DADD";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be unused. Noone lowers to this node, please remove and make dadd instruction basically patternless.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This extra case is needed just to make a compiler happy. Otherwise it reports a warning about enumeration value not handled in switch statement.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Because you added the enumeration value that is completely unused. For remove I mean the whole DADD node.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then I'll need to change the definition

defm DADD : Arith<0b1010, "dadd", MSP430dadd, 1, [SR]>;

which uses

def MSP430dadd    : SDNode<"MSP430ISD::DADD", SDT_MSP430DAdd, []>;

Should I introduce a new patternless Arith multiclass just to define dadd?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok, I see. Then let's keep the current code. But please add the comment that nothing generates this node yet :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could have e.g. a target intrinsic / builtin for this actually!

asl
asl reviewed Oct 16, 2018
View reviewed changes
src/llvm/lib/Target/MSP430/MSP430ISelLowering.cpp
@@ -940,18 +940,7 @@ SDValue MSP430TargetLowering::LowerShifts(SDValue Op,

// Expand non-constant shifts to loops:
Copy link
Contributor

@asl asl Oct 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it performed by generic code now? If yes, what do we generate here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generated code stays the same. For example, ashr16 from test/CodeGen/MSP430/shifts.ll:

ashr16:                                 ; @ashr16
; %bb.0:                                ; %entry
        cmp.b   #0, r13
        jeq     .LBB4_2
.LBB4_1:                                ; %entry
                                        ; =>This Inner Loop Header: Depth=1
        rra     r12
        sub.b   #1, r13
        jne     .LBB4_1
.LBB4_2:                                ; %entry
        ret

@asl asl merged commit 87378e1 into access-softek:mainline Oct 17, 2018
asl added a commit that referenced this pull request Oct 17, 2018
@asl
Merge pull request #1 from mskvortsov/pr-mclayer
52a175f 
Add MC layer for MSP430
@mskvortsov mskvortsov deleted the pr-mclayer branch October 18, 2018 07:30
@asl asl mentioned this pull request Oct 23, 2018
Closed
asl pushed a commit that referenced this pull request Nov 26, 2018
@alexfh
Fix uninitialized variable.
b81719b 
Flags variable was not initialized and later used (both isMBBSafeToOutlineFrom
implementations assume it's initialized), which breaks
test/CodeGen/AArch64/machine-outliner.mir. under memory sanitizer:
MemorySanitizer: use-of-uninitialized-value
    #0  in llvm::AArch64InstrInfo::getOutliningType(llvm::MachineInstrBundleIterator<llvm::MachineInstr, false>&, unsigned int) const llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5494:9
    #1  in (anonymous namespace)::InstructionMapper::convertToUnsignedVec(llvm::MachineBasicBlock&, llvm::TargetInstrInfo const&) llvm/lib/CodeGen/MachineOutliner.cpp:772:19
    #2  in (anonymous namespace)::MachineOutliner::populateMapper((anonymous namespace)::InstructionMapper&, llvm::Module&, llvm::MachineModuleInfo&) llvm/lib/CodeGen/MachineOutliner.cpp:1543:14
    #3  in (anonymous namespace)::MachineOutliner::runOnModule(llvm::Module&) llvm/lib/CodeGen/MachineOutliner.cpp:1645:3
    #4  in (anonymous namespace)::MPPassManager::runOnModule(llvm::Module&) llvm/lib/IR/LegacyPassManager.cpp:1744:27
    #5  in llvm::legacy::PassManagerImpl::run(llvm::Module&) llvm/lib/IR/LegacyPassManager.cpp:1857:44
    #6  in compileModule(char**, llvm::LLVMContext&) llvm/tools/llc/llc.cpp:597:8

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@346761 91177308-0d34-0410-b5e6-96231b3b80d8
asl pushed a commit that referenced this pull request Nov 26, 2018
@LebedevRI
[clang][CodeGen] Implicit Conversion Sanitizer: discover the world of…
d8a18b8 
… CompoundAssign operators

Summary:
As reported by @regehr (thanks!) on twitter (https://twitter.com/johnregehr/status/1057681496255815686),
we (me) has completely forgot about the binary assignment operator.
In AST, it isn't represented as separate `ImplicitCastExpr`'s,
but as a single `CompoundAssignOperator`, that does all the casts internally.
Which means, out of these two, only the first one is diagnosed:
```
auto foo() {
    unsigned char c = 255;
    c = c + 1;
    return c;
}
auto bar() {
    unsigned char c = 255;
    c += 1;
    return c;
}
```
https://godbolt.org/z/JNyVc4

This patch does handle the `CompoundAssignOperator`:
```
int main() {
  unsigned char c = 255;
  c += 1;
  return c;
}
```
```
$ ./bin/clang -g -fsanitize=integer /tmp/test.c && ./a.out
/tmp/test.c:3:5: runtime error: implicit conversion from type 'int' of value 256 (32-bit, signed) to type 'unsigned char' changed the value to 0 (8-bit, unsigned)
    #0 0x2392b8 in main /tmp/test.c:3:5
    #1 0x7fec4a612b16 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x22b16)
    #2 0x214029 in _start (/build/llvm-build-GCC-release/a.out+0x214029)
```

However, the pre/post increment/decrement is still not handled.

Reviewers: rsmith, regehr, vsk, rjmccall, #sanitizers

Reviewed By: rjmccall

Subscribers: mclow.lists, cfe-commits, regehr

Tags: #clang, #sanitizers

Differential Revision: https://reviews.llvm.org/D53949

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@347258 91177308-0d34-0410-b5e6-96231b3b80d8
@dmikushin dmikushin mentioned this pull request Jan 24, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asl asl asl left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mskvortsov @asl