This is an archive of the discontinued LLVM Phabricator instance.

[Sparc] Leon errata fixes passes.
AbandonedPublic

Authored by lero_chris on Jul 3 2016, 11:17 AM.

Details

Summary

Errata fixes for various errata in different versions of the Leon variants of the Sparc 32 bit processor.

The nature of the errata are listed in the comments preceding the errata fix passes. Relevant unit tests are implemented for each of these.

Note: Running clang-format has changed a few other lines too, unrelated to the implemented errata fixes. These have been left in as this keeps the code formatting consistent.

Diff Detail

Repository
rL LLVM

Event Timeline

lero_chris updated this revision to Diff 62632.Jul 3 2016, 11:17 AM
lero_chris retitled this revision from to [Sparc] Leon errata fixes passes..
lero_chris updated this object.
lero_chris added a reviewer: jyknight.
lero_chris set the repository for this revision to rL LLVM.
lero_chris added a subscriber: llvm-commits.
lero_chris updated this object.Jul 3 2016, 11:20 AM
jacob_hansen edited edge metadata.Jul 4 2016, 7:50 AM

Can't seem to find tests for the following passes:

FlushCacheLineSWAP
IgnoreZeroFlag
FixCALL

llvm/lib/Target/Sparc/LeonFeatures.td
18 ↗(On Diff #62632)

Is this feature definition needed/used anywhere? I think the previously defined sofware floating point definition in Sparc.td is enough.

46 ↗(On Diff #62632)

these requirement comments should probably be removed (here as well as in the descriptions).

llvm/lib/Target/Sparc/LeonPasses.cpp
654 ↗(On Diff #62632)

Remove outcommented code.

714 ↗(On Diff #62632)

would it make sense for the compiler to output a warning when this pass is successfully removing the call? The developer may want to know that the rounding mode change is not being executed correctly.

llvm/test/CodeGen/SPARC/LeonPreventRoundChangeUT.ll
3 ↗(On Diff #62632)

is this check correct? Shouldn't it rather check that there is no call to fesetround ?

lero_chris updated this revision to Diff 63195.Jul 8 2016, 3:59 AM
lero_chris edited edge metadata.

Update to address change recommended by comments. Small formatting changes. Missing unit tests added.

dcederman added inline comments.
lib/Target/Sparc/LeonFeatures.td
65–68 ↗(On Diff #63195)

As jacob_hansen commented, this should only be a warning. It is not a good idea to remove valid function calls without notifying the user.

81–85 ↗(On Diff #63195)

Flushing the cache is not a valid workaround for this errata, so this feature is not needed and will only decrease performance.

lib/Target/Sparc/LeonPasses.h
162 ↗(On Diff #63195)

LBR35 is not the official name of the errata or errata fix, it should be removed to avoid confusion.

lib/Target/Sparc/Sparc.td
144 ↗(On Diff #63195)

UMAC/SMAC is not available on ut699, gr712rc, or gr740.

lib/Target/Sparc/SparcISelLowering.cpp
1417–1419 ↗(On Diff #63195)

This code has been commented out.

3100 ↗(On Diff #63195)

This does not seem related to the errata fixes.

lero_chris updated this revision to Diff 63215.Jul 8 2016, 7:02 AM

Further updates to address some review comments.

This revision was automatically updated to reflect the committed changes.
jyknight edited edge metadata.Jul 8 2016, 9:23 AM

I see you've already committed this in the time it took me to write down comments this morning. Please revert, and reopen the review. It seems to me that there are significant issues here. Some of those also present in code in LeonPasses.cpp that already was committed.

Also:

While it's not a big deal in this case, please don't do a file-wide clang-format in the same change as actual changes. You should be doing "git clang-format" or "clang-format-diff" to only format the lines you're modifying. (If the file is really egregiously ill-formatted, you can also run clang-format on the unmodified file, and commit that first).

lib/Target/Sparc/LeonFeatures.td
65–68 ↗(On Diff #63195)

This doesn't appear to have been addressed.

81–85 ↗(On Diff #63195)

This doesn't appear to have been addressed.

lib/Target/Sparc/LeonPasses.cpp
263 ↗(On Diff #63215)

All of the isInlineAsm branches -- new and old -- seem wrong.

Testing instructions by looking at the prefix of the asm string can't possibly work properly. E.g., it's completely wrong if there are multiple instructions in an inline asm string.

One of two things makes sense. Either:
a) these fixes are only intended to be done on compiler-generated code. If so just remove the isInlineAsm clauses.

b) it's supposed to be done at translation of asm into object file. In which case, it should be done for an actual assembly input file too and these transforms are misplaced here.

I don't know myself how to introduce a transform at the asm level; there doesn't seem to be an obvious way to do it. Please ask for advice on the mailing list if that's what you need to do.

lib/Target/Sparc/SparcISelLowering.cpp
1642 ↗(On Diff #63215)

This should actually have been 32. 64bit atomic instructions are not supported on leon. Also, unrelated to this change.

llvm/trunk/lib/Target/Sparc/SparcISelLowering.cpp
3380 ↗(On Diff #63234)

Unrelated change.

3458 ↗(On Diff #63234)

Unrelated change.

lero_chris reopened this revision.Jul 8 2016, 9:31 AM

Re-opened to address latest comments.

Re-opened to address latest comments.

Please revert the change, too.

lero_chris updated this revision to Diff 67419.EditedAug 9 2016, 2:57 PM
lero_chris edited edge metadata.

Addressing issues in previous review.

  • Inline assembler removed.
  • Non-working optimisation removed.
  • Warning diagnostic generated on change of instruction behaviour for erratum fix.
  • Changes to Unit tests to remove tests that work on inline assembler behaviour.
  • Added (2) missing errata fixes.
lero_chris updated this revision to Diff 67420.Aug 9 2016, 2:59 PM

Removed change to CMakeLists.txt that was relevant to a different change and inadvertently added to this review.

I noticed that some of the files have been marked as executable. You should change that before committing anything.

lib/Target/Sparc/LeonPasses.cpp
757–762 ↗(On Diff #67420)

How does this fill the data cache?

851–859 ↗(On Diff #67420)

Erratas in trap handling should not be handled by the compiler, it should be done manually in each OS. Did you try to compile the trap handling code in, for example, Linux or RTEMS with this fix?

lib/Target/Sparc/LeonPasses.h
107–109 ↗(On Diff #67420)

What is LBR29? Please add a description of the errata and the fix or a reference to an official document with the information.

What do you mean by application startup?

lero_chris updated this revision to Diff 67733.Aug 11 2016, 1:02 PM

Modified some unit tests that were using inline assembler tests to work without this to reflect the non-use of inline assembler in the modified passes.

Removed "LBR" references to errata fixes as these were internal project references.

Addressing review comments.

lib/Target/Sparc/LeonFeatures.td
66–69 ↗(On Diff #67733)

Warning now issued when this pass makes any changes.

82–86 ↗(On Diff #67733)

Removed this pass if it does nothing.

lib/Target/Sparc/LeonPasses.cpp
231–232 ↗(On Diff #67733)

All inline assembly code removed.

757–762 ↗(On Diff #67733)

A cache stores recently-accessed data. To fill the cache, we need to access data big enough to fill the L1 cache of the LEON 2 FT CPU. To solve this problem, we use NOP instructions to fill a block of memory with NOP instructions. The LEON 2’s L1 data cache is 16 KB [6] and the size of the NOP instruction is 4 Bytes. Therefore, we need 4096 NOPs to entirely fill the cache. Note this will happen only once at the first call of the runOnMachineFunction() method.

851–859 ↗(On Diff #67733)

Do you suggest that this pass should not be part of the compiler then? I can remove it if it fixes nothing.

lib/Target/Sparc/LeonPasses.h
108–110 ↗(On Diff #67733)

Removed these internal project reference codes.

188 ↗(On Diff #67733)

Removed these internal project reference codes.

lib/Target/Sparc/Sparc.td
145 ↗(On Diff #67733)

Removed from feature set.

dcederman edited edge metadata.Aug 12 2016, 2:42 AM

Some more inline comments.

Could you please also revert the committed changes, like @jyknight asked you?

lib/Target/Sparc/LeonFeatures.td
88–90 ↗(On Diff #67733)

A cache miss occurs when the data at the memory address accessed is not available in the cache. Filling the data cache with random data that will not be accessed again will not prevent cache misses, it will make them more likely.

Could you reference the document that describes the errata you are trying to provide a fix for? I looked in http://www.atmel.com/Images/doc4409.pdf but could not find a match.

lib/Target/Sparc/LeonPasses.cpp
757–762 ↗(On Diff #67733)

But you are not accessing data? It is just a loop that does nothing. And if I understand the code correctly, you randomly add it to the first function you find?

851–859 ↗(On Diff #67733)

You should remove it. Automatically changing assembly code in the trap handler is extremely risky.

This revision was automatically updated to reflect the committed changes.
dcederman reopened this revision.Aug 12 2016, 4:22 AM

Why did you commit your changes mid-review? I'm reopening this review.

Hi Daniel. Nobody was commenting and I need to get this checked-in as I won't be around for three weeks now. You commented at *exactly* the same time as I did the check-in. Otherwise, I had addressed all the review comments.

I'll look at the comments and make any necessary changes when I am next at work.

Hi Daniel. Nobody was commenting and I need to get this checked-in as I won't be around for three weeks now. You commented at *exactly* the same time as I did the check-in. Otherwise, I had addressed all the review comments.

I'll look at the comments and make any necessary changes when I am next at work.

This is not OK. You have ignored two requests for a revert in a row now. I have reverted all of this in r278511.

lero_chris updated this revision to Diff 67838.Aug 12 2016, 8:01 AM
lero_chris edited edge metadata.

Removed RestoreExecAddress and FillDataCache passes as these will have no effect.

Hi Daniel. Nobody was commenting and I need to get this checked-in as I won't be around for three weeks now. You commented at *exactly* the same time as I did the check-in. Otherwise, I had addressed all the review comments.

I'll look at the comments and make any necessary changes when I am next at work.

This is not OK. You have ignored two requests for a revert in a row now. I have reverted all of this in r278511.

Sorry James. I had been addressing the comments. I hadn't realised that "revert" meant rolling back the repository changes too. SVN isn't something I use much and the language differs somewhat between source control systems.

I'll abandon this review and start a new review, albeit that this is all working and tested now.

lero_chris abandoned this revision.Aug 12 2016, 8:13 AM

Abandoned.

llvm/trunk/test/CodeGen/SPARC/LeonFillDataCachePassUT.ll