Page MenuHomePhabricator

TargetMachine: Indicate whether machine verifier passes.
ClosedPublic

Authored by MatzeB on May 30 2017, 2:54 PM.

Details

Summary

This adds a callback to the LLVMTargetMachine that lets target indicate
that they do not pass the machine verifier checks in all cases yet.

This is intended to be a temporary measure while the targets are fixed
allowing us to enable the machine verifier by default in expensive
checks settings!

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB created this revision.May 30 2017, 2:54 PM

MSP430 was fixed, it should pass the tests now.

MatzeB updated this revision to Diff 100784.May 30 2017, 3:03 PM

MSP430 was fixed, it should pass the tests now.

Great, I removed the overload!

jlebar added a subscriber: jlebar.May 30 2017, 3:50 PM

lgtm for NVPTX changes.

fhahn edited edge metadata.May 30 2017, 3:53 PM

Looks good, thanks for picking that up. Unfortunately I did not have any time to work on the machine verification pass recently.

Looks good for Lanai changes

LGTM for MIPS changes.

tstellar added inline comments.
lib/Target/AMDGPU/AMDGPUTargetMachine.h
73–75 ↗(On Diff #100784)

I think GCNTargetMachine should be able to return true for this, did you try this?

filcab added a subscriber: filcab.May 31 2017, 4:21 AM

Should we add TODO comments to all those overrides and state that the errors should be fixed and the overrides (and the function itself, afterwards) be removed?

RKSimon edited edge metadata.May 31 2017, 5:05 AM

x86 looks good.

Probably the most pragmatic solution for now, I'm just concerned that this will lead to some targets never getting fixed......

x86 looks good.

Probably the most pragmatic solution for now, I'm just concerned that this will lead to some targets never getting fixed......

Indeed I also have some worries that that callback will never go away once introduced. However I find the status quo where pretty much all targets are broken even more worrying and I am optimistic that this function acts more as a motivation and goal when people see that other targets are clean and theirs isn't.

Should we add TODO comments to all those overrides and state that the errors should be fixed and the overrides (and the function itself, afterwards) be removed?

I will add a comment that the function is a stopgap measure and will be removed. My plan for the targets is to file PRs against them which I hope works better than TODOs in the source.

lib/Target/AMDGPU/AMDGPUTargetMachine.h
73–75 ↗(On Diff #100784)

No I have not tested the individual AMDGPU target machines, I'll try.

RKSimon added inline comments.May 31 2017, 11:03 AM
include/llvm/Target/TargetMachine.h
305 ↗(On Diff #100784)

Would it be too strict to add a comment saying that targets should not become 'unclean' again once they are passing expensive checks?

lib/Target/AVR/AVRTargetMachine.h
46 ↗(On Diff #100784)

FYI I'm not sure we have any EXPENSIVE_CHECKS buildbot that enables AVR (or WebAssembly).

lib/Target/Hexagon/HexagonTargetMachine.h
49 ↗(On Diff #100784)

Hexagon was clean on my last set of tests (PR32146 / PR33048)

This revision was automatically updated to reflect the committed changes.
dylanmckay added inline comments.
lib/Target/AVR/AVRTargetMachine.h
46 ↗(On Diff #100784)

Can confirm in the AVR case.

MatzeB added inline comments.May 31 2017, 4:25 PM
lib/Target/AVR/AVRTargetMachine.h
46 ↗(On Diff #100784)

No I don't think we have. I also added this return false; defensively feel free to remove it if AVR is clean.

BTW: You can always manually run all lit tests with verify-machineinstrs with llvm-lit -Dllc="llc -verify-machineinstrs" that saves you the time to recompile with EXPENSIVE_CHECKS enabled.