[PATCH] D123515: [RISCV] Support '.option arch' directive

[luxufan via Phabricator via llvm-commits]

StephenFan added inline comments.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2498
+          if (getSTI().hasFeature(Feature.Value) &&
+              Feature.Implies.test(Ext->Value))
+            return Error(Parser.getTok().getLoc(),
----------------
craig.topper wrote:
> craig.topper wrote:
> > StephenFan wrote:
> > > kito-cheng wrote:
> > > > StephenFan wrote:
> > > > > craig.topper wrote:
> > > > > > I don't think the implies list is normalized to include transitive implications. For example if A implies B and B implies C. I don't think the C is A's implies list.
> > > > > > 
> > > > > > This also doesn't cover cases that can't be expressed in the .td file. At one point Zve32f required either F or Zfinx. But F and Zfinx are mutually exclusive so we could't have Zve32f imply either of them in the .td file. All we could do is check it in `RISCVISAInfo::checkDependency`.
> > > > > > I don't think the implies list is normalized to include transitive implications. For example if A implies B and B implies C. I don't think the C is A's implies list.
> > > > > > 
> > > > > Can this be handled by RISCVISAInfo? When I parse RISCVIAInfo from featureBits, it always updates implication.
> > > > > 
> > > > > 
> > > > Yeah, use RISCVISAInfo should be reasonable, no reason to duplicate and maintain the logic here :)
> > > > 
> > > > An idea thought here:
> > > > - Convert feature bits to feature string vector
> > > > - Convert extension list to a vector<string>
> > > > - Implement a new parse function `static llvm::Expected< std::unique_ptr< RISCVISAInfo > > RISCVISAInfo::parseArchOption (const std::vector< StringRef > &BaseFeatures, const std::vector< StringRef > &ExtensionList)`, then handle all ISA stuff logic here.
> > > > I don't think the implies list is normalized to include transitive implications. For example if A implies B and B implies C. I don't think the C is A's implies list.
> > > > 
> > > Thanks, @craig.topper  and @kito-cheng . But it does check for transitivity here. And I added a test of this:
> > > ```
> > > .option arch, +d, -zicsr
> > > ```
> > > llvm-mc can report an error correctly. So there is no functional mistake and if we want to use RISCVISAInfo, we just need a NFC patch.
> > > > This also doesn't cover cases that can't be expressed in the .td file. At one point Zve32f required either F or Zfinx. But F and Zfinx are mutually exclusive so we could't have Zve32f imply either of them in the .td file. All we could do is check it in `RISCVISAInfo::checkDependency`.
> > > Do you mean like that we can't enable the `zfinx` extension if `f` extension exists? If so, I have addressed it.
> > > 
> > > > I don't think the implies list is normalized to include transitive implications. For example if A implies B and B implies C. I don't think the C is A's implies list.
> > > > 
> > > Thanks, @craig.topper  and @kito-cheng . But it does check for transitivity here. And I added a test of this:
> > > ```
> > > .option arch, +d, -zicsr
> > > ```
> > > llvm-mc can report an error correctly. So there is no functional mistake and if we want to use RISCVISAInfo, we just need a NFC patch.
> > 
> > I guess it works because F was implied by D so the check was applied directly against F when Zicsr was disabled.
> > 
> > > > This also doesn't cover cases that can't be expressed in the .td file. At one point Zve32f required either F or Zfinx. But F and Zfinx are mutually exclusive so we could't have Zve32f imply either of them in the .td file. All we could do is check it in `RISCVISAInfo::checkDependency`.
> > > Do you mean like that we can't enable the `zfinx` extension if `f` extension exists? If so, I have addressed it.
> > > 
> > 
> > I wasn't specifically referring to that. I was only saying that the implies list in the .td file can be incomplete.
> The .td file for Zve32f requiring F was only updated recently here https://reviews.llvm.org/D150021
> > > I don't think the implies list is normalized to include transitive implications. For example if A implies B and B implies C. I don't think the C is A's implies list.
> > > 
> > Thanks, @craig.topper  and @kito-cheng . But it does check for transitivity here. And I added a test of this:
> > ```
> > .option arch, +d, -zicsr
> > ```
> > llvm-mc can report an error correctly. So there is no functional mistake and if we want to use RISCVISAInfo, we just need a NFC patch.
> 
> I guess it works because F was implied by D so the check was applied directly against F when Zicsr was disabled.
Yeah, it works because F is implied when D is enabled. And when we were checking zicsr, F's imply list was checked. So llvm-mc would report an error.
Maybe I have not got your point about " the implies list in the .td file can be incomplete.". Would you mind giving me an assembly example to show my mistake?




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123515/new/

https://reviews.llvm.org/D123515