This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] writeNopData support generate c.nop
ClosedPublic

Authored by shiva0217 on Dec 13 2017, 7:10 PM.

Details

Summary

The assembler will generate nop for alignment. For the riscv target support C extension, it may need to generate c.nop to fix up alignment.

Diff Detail

Repository
rL LLVM

Event Timeline

shiva0217 created this revision.Dec 13 2017, 7:10 PM
asb added inline comments.Dec 14 2017, 2:29 AM
lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
37 ↗(On Diff #126887)

Is there any way we can avoid spreading parsing logic throughout the backend? Can we get hold of an MCSubtargetInfo and check for STI.getFeatureBits()[RISCV::FeatureStdExtC]? The change to Triple.cpp then wouldn't be necessary in this patch (though might be something we want to look at, to make arguments more uniform between clang and the llvm tools).

103 ↗(On Diff #126887)

I think it's kind of confusing define i here, then using it below, but also shadowing it with a new i in the NumOps loop. Would it be clearer to just calculate Nop16Count and Nop32Count, then decrement those values directly?

111 ↗(On Diff #126887)

Indentation.

shiva0217 updated this revision to Diff 127299.Dec 17 2017, 9:44 PM
shiva0217 edited the summary of this revision. (Show Details)
shiva0217 added inline comments.Dec 17 2017, 9:59 PM
lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
37 ↗(On Diff #126887)

The latest revision will:

  1. Add parseRISCVTriple to get target feature by parsing triple for the backend.
  2. Add MCSubtargetInfo in RISCVAsmBackend class and using STI.getFeatureBits()[RISCV::FeatureStdExtC] to check target feature.
  3. The STI initial by parsing triple, if we remove the change in Triple.cpp, triple with suffix imac will not allow, and then STI can't get target feature.
103 ↗(On Diff #126887)

Yes, define Nop32Count and Nop16Count would be more readable.
Thanks.

shiva0217 edited the summary of this revision. (Show Details)Dec 17 2017, 10:00 PM
asb added a comment.Dec 18 2017, 2:29 AM

Creating a TgtMCSubtargetInfo without passing CPUName and FeatureString is what ARM does in ARMAsmBackend, but it seems pretty confusing. The fact that the desired behaviour doesn't work with -triple riscv32 -mattr=+c indicates to me that something is wrong.

D20830 added an MCSubtargetInfo parameter to relaxInstruction. I could see that the same justification could be used to add MCSubtargetInfo to writeNopData.

Does anyone else have views on this? It's been a while since I stepped through the TargetRegistry code - if feasible, having a properly initialised RISCVMCSubtargetInfo (with cpustring and featurestring set correctly) as a field of RISCVAsmBackend seems ideal, but from an initial look it might be difficult.

Hi Alex, I think https://reviews.llvm.org/D41349 is the right thing to do, then we could get correct MCSubtargetInfo by -mattr in MCAsmBackend class, thanks for your work.
We could remove the parsing logic in this patch once https://reviews.llvm.org/D41349 landing.

shiva0217 updated this revision to Diff 128496.Jan 3 2018, 1:29 AM
shiva0217 edited the summary of this revision. (Show Details)

Remove parsing logic and get MCSubtargetInfo directly due to https://reviews.llvm.org/D41349 has been landing.

asb added a comment.Jan 12 2018, 1:09 AM

Thanks for the update Shiva - a few minor comments in-line

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
94–100 ↗(On Diff #128496)

This has slightly more nesting than normal LLVM style prefers. It could may be rewritten as:

// Nops in the base RISC-V ISA must be 4 bytes, but the compressed extension 
// introduces 2 byte nops.
if (!HasStdExtC && (Count % 4) != 0)
  return false;
if (HasStdExtC && (Count % 2) != 0)
  return false;

Probably better would be to change the function entry to this:

bool HasStdExtC = STI.getFeatureBits()[RISCV::FeatureStdExtC];
unsigned MinNopLen = HasStdExtC ? 2 : 4;

if ((Count % MinNopLen) != 0)
  return false;
101 ↗(On Diff #128496)

Nit: end comment with full stop.

104 ↗(On Diff #128496)

Should have two spaces of indentation rather than four.

106 ↗(On Diff #128496)

Nit: end comment with full stop.

shiva0217 updated this revision to Diff 129616.Jan 12 2018, 6:26 AM

Update the patch to reflect the comments.

asb accepted this revision.Jan 17 2018, 6:18 AM

Thanks! Looks good to me.

This revision is now accepted and ready to land.Jan 17 2018, 6:18 AM
This revision was automatically updated to reflect the committed changes.