This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][TableGen] Skip tied result operands for InstAlias
ClosedPublic

Authored by huntergr on Jan 27 2017, 3:18 AM.

Details

Summary

This patch checks the number of operands in the resulting
instruction instead of just the alias, then skips over
tied operands when generating the printing method.

This allows us to generate the preferred assembly syntax
for the AArch64 'ins' instruction, which should always be
displayed as 'mov' according to the ARMARM.

Several unit tests have changed as a result, but only to
reflect the preferred disassembly.

Some other InstAlias patterns (movk/bic/orr) needed a
slight adjustment to stop them becoming the default
and breaking other unit tests.

Diff Detail

Repository
rL LLVM

Event Timeline

huntergr created this revision.Jan 27 2017, 3:18 AM

Hi Graham,

I think you got your patch backwards...

cheers,
--renato

huntergr updated this revision to Diff 86043.Jan 27 2017, 4:44 AM

Hi Graham,

This looks ok to me, but I'm not confident to approve a TableGen back-end patch that could potentially change the behaviour in other targets.

I'm adding other people that touched the code / table files recently, and I'm hoping that other targets developers got the mail via the commits list and will chime in if necessary.

cheers,
--renato

lib/Target/AArch64/AArch64InstrInfo.td
455 ↗(On Diff #86043)

If I read Target.td correctly, this completely disables the printing in all cases, not just the one you're testing.

4399 ↗(On Diff #86043)

I'm not sure why this was disabled, probably due to a similar conflict in the past.

The original patch that introduced this by Tim explains nothing, so I'll assume this is ok.

Hi Graham,

This looks ok to me, but I'm not confident to approve a TableGen back-end patch that could potentially change the behaviour in other targets.

I'm adding other people that touched the code / table files recently, and I'm hoping that other targets developers got the mail via the commits list and will chime in if necessary.

cheers,
--renato

Thanks for looking over it.

lib/Target/AArch64/AArch64InstrInfo.td
455 ↗(On Diff #86043)

That's my understanding as well. In this case, imm0_65535 printed in hex, whereas the unit tests are in decimal. The alternative is to change the tests to expect hex.

rengolin added inline comments.Jan 27 2017, 7:20 AM
lib/Target/AArch64/AArch64InstrInfo.td
455 ↗(On Diff #86043)

No, decimals are fine.

It may be that the other encodings are also just for the assembler, not code generation, so would be better to have the explicit 0.

But I don't think this is worth doing in this patch. It would be better for someone to look at that later with more extensive testing, to make sure nothing is lost in translation.

Ping.

For clarity wrt. the movk/bic/orr pattern changes, they never triggered when printing asm before because the base patterns in AArch64InstrFormats.td (BaseInsertImmediate and BaseSIMDModifiedImmVectorTied) both contain tied register constraints. Since they now print with this change, I've adjusted the priority where needed to avoid breaking the existing unit tests, and just change ins to mov.

Ping.

Hi Graham,

I was waiting from other comments, especially regarding the tied operand logic for other targets, which I'm not familiar with. I'm surprised this didn't trigger anything else in any other target. Are you building and testing all targets?

For clarity wrt. the movk/bic/orr pattern changes, they never triggered when printing asm before because the base patterns in AArch64InstrFormats.td (BaseInsertImmediate and BaseSIMDModifiedImmVectorTied) both contain tied register constraints. Since they now print with this change, I've adjusted the priority where needed to avoid breaking the existing unit tests, and just change ins to mov.

I suspected as much. It looks good from the AArch64 point of view. We can give it a few more days, and if no one objects, I'll approve and we try our luck on the buildbots. :)

cheers,
--renato

Ping.

Hi Graham,

I was waiting from other comments, especially regarding the tied operand logic for other targets, which I'm not familiar with. I'm surprised this didn't trigger anything else in any other target. Are you building and testing all targets?

I've run make check-all for all targets, no failures; I was a little surprised that no changes were required on other targets as well (with both debug and release builds), but I don't know how many other architectures have significant differences between representations of the same instruction.

For clarity wrt. the movk/bic/orr pattern changes, they never triggered when printing asm before because the base patterns in AArch64InstrFormats.td (BaseInsertImmediate and BaseSIMDModifiedImmVectorTied) both contain tied register constraints. Since they now print with this change, I've adjusted the priority where needed to avoid breaking the existing unit tests, and just change ins to mov.

I suspected as much. It looks good from the AArch64 point of view. We can give it a few more days, and if no one objects, I'll approve and we try our luck on the buildbots. :)

cheers,
--renato

rengolin accepted this revision.Feb 6 2017, 5:28 AM

I've run make check-all for all targets, no failures; I was a little surprised that no changes were required on other targets as well (with both debug and release builds), but I don't know how many other architectures have significant differences between representations of the same instruction.

Ha, right, this was introduced by Tim specifically for AArch64 in commit r208880, and he moved the x86 usage of this code away, so it should only affect the AArch64 back-end.

I'm setting to accepted now, but wait a day to commit, so that we give a bit more time for people to review.

cheers,
--renato

This revision is now accepted and ready to land.Feb 6 2017, 5:28 AM
This revision was automatically updated to reflect the committed changes.
MatzeB added a subscriber: MatzeB.Feb 8 2017, 10:43 AM

Hi,

the "Clang Stage 2: cmake, R -g Asan+Ubsan" buildbot on greendragon is failing as of build
#3583. Full log at http://lab.llvm.org:8080/green/job/clang-stage2-cmake-RgSan_build/3583/consoleFull#-1431883905a1ca8a51-895e-46c6-af87-ce24fa4cd561

Given the backtrace this seems like the most likely commit in the change list. Could you please take a look.

Thanks,

Matthias

FAILED: lib/Target/AArch64/AArch64GenAsmWriter.inc.tmp

cd /Users/buildslave/jenkins/sharedspace/clang-stage2-cmake-RgSan@2/clang-build/lib/Target/AArch64 && /Users/buildslave/jenkins/sharedspace/clang-stage2-cmake-RgSan@2/clang-build/bin/llvm-tblgen -gen-asm-writer -I /Users/buildslave/jenkins/sharedspace/clang-stage2-cmake-RgSan@2/llvm/lib/Target/AArch64 -I /Users/buildslave/jenkins/sharedspace/clang-stage2-cmake-RgSan@2/llvm/include -I /Users/buildslave/jenkins/sharedspace/clang-stage2-cmake-RgSan@2/llvm/lib/Target /Users/buildslave/jenkins/sharedspace/clang-stage2-cmake-RgSan@2/llvm/lib/Target/AArch64/AArch64.td -o /Users/buildslave/jenkins/sharedspace/clang-stage2-cmake-RgSan@2/clang-build/lib/Target/AArch64/AArch64GenAsmWriter.inc.tmp

309==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x615000f8a4e4 at pc 0x00010916bae6 bp 0x7fff56b25fb0 sp 0x7fff56b25fa8

READ of size 4 at 0x615000f8a4e4 thread T0

#0 0x10916bae5 in (anonymous namespace)::AsmWriterEmitter::EmitPrintAliasInstruction(llvm::raw_ostream&) AsmWriterEmitter.cpp:835
#1 0x1091514fa in llvm::EmitAsmWriter(llvm::RecordKeeper&, llvm::raw_ostream&) AsmWriterEmitter.cpp:1125
#2 0x1094b145d in (anonymous namespace)::LLVMTableGenMain(llvm::raw_ostream&, llvm::RecordKeeper&) TableGen.cpp:124
#3 0x10956b99e in llvm::TableGenMain(char*, bool (*)(llvm::raw_ostream&, llvm::RecordKeeper&)) Main.cpp:109
#4 0x1094b0d30 in main TableGen.cpp:205
#5 0x7fff95d095ac in start (libdyld.dylib:x86_64+0x35ac)

0x615000f8a4e4 is located 28 bytes to the left of 504-byte region [0x615000f8a500,0x615000f8a6f8)
allocated by thread T0 here:

#0 0x109c0ee1b in wrap__Znwm (libclang_rt.asan_osx_dynamic.dylib:x86_64+0x62e1b)
#1 0x10922e41a in std::__1::__split_buffer<llvm::CGIOperandList::OperandInfo, std::__1::allocator<llvm::CGIOperandList::OperandInfo>&>::__split_buffer(unsigned long, unsigned long, std::__1::allocator<llvm::CGIOperandList::OperandInfo>&) new:215
#2 0x10921e655 in std::__1::vector<llvm::CGIOperandList::OperandInfo, std::__1::allocator<llvm::CGIOperandList::OperandInfo> >::reserve(unsigned long) __split_buffer:310
#3 0x10921ad71 in llvm::CGIOperandList::CGIOperandList(llvm::Record*) CodeGenInstruction.cpp:53
#4 0x10922205e in llvm::CodeGenInstruction::CodeGenInstruction(llvm::Record*) CodeGenInstruction.cpp:28
#5 0x1092ffda0 in llvm::CodeGenTarget::ReadInstructions() const STLExtras.h:846
#6 0x1093002ee in llvm::CodeGenTarget::ComputeInstrsByEnum() const CodeGenTarget.h:150
#7 0x10910fd6f in llvm::CodeGenTarget::getInstructionsByEnumValue() const CodeGenTarget.h:166
#8 0x1091506df in llvm::EmitAsmWriter(llvm::RecordKeeper&, llvm::raw_ostream&) AsmWriterEmitter.cpp:1113
#9 0x1094b145d in (anonymous namespace)::LLVMTableGenMain(llvm::raw_ostream&, llvm::RecordKeeper&) TableGen.cpp:124
#10 0x10956b99e in llvm::TableGenMain(char*, bool (*)(llvm::raw_ostream&, llvm::RecordKeeper&)) Main.cpp:109
#11 0x1094b0d30 in main TableGen.cpp:205
#12 0x7fff95d095ac in start (libdyld.dylib:x86_64+0x35ac)

SUMMARY: AddressSanitizer: heap-buffer-overflow AsmWriterEmitter.cpp:835 in (anonymous namespace)::AsmWriterEmitter::EmitPrintAliasInstruction(llvm::raw_ostream&)
Shadow bytes around the buggy address:

0x1c2a001f1440: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x1c2a001f1450: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x1c2a001f1460: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x1c2a001f1470: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x1c2a001f1480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 fa

>0x1c2a001f1490: fa fa fa fa fa fa fa fa fa fa fa fa[fa]fa fa fa

0x1c2a001f14a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x1c2a001f14b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x1c2a001f14c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x1c2a001f14d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 fa
0x1c2a001f14e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa

Shadow byte legend (one shadow byte represents 8 application bytes):

Addressable:           00
Partially addressable: 01 02 03 04 05 06 07 
Heap left redzone:       fa
Freed heap region:       fd
Stack left redzone:      f1
Stack mid redzone:       f2
Stack right redzone:     f3
Stack after return:      f5
Stack use after scope:   f8
Global redzone:          f9
Global init order:       f6
Poisoned by user:        f7
Container overflow:      fc
Array cookie:            ac
Intra object redzone:    bb
ASan internal:           fe
Left alloca redzone:     ca
Right alloca redzone:    cb

309==ABORTING