[PATCH] D119541: [RISCV] Fix RISCVTargetInfo::initFeatureMap, add non-ISA features back after implication

[Alex Bradbury via Phabricator via cfe-commits]

asb added a comment.

In D119541#3940597 , @arichardson 
wrote:

> I just noticed that target features (e.g. -mrelax) are broken in all LLVM 14 
> releases due to D113336  . This should have 
> been cherry-picked back tot the release branch, but it's too late now. In the 
> future please ensure that important fixes such as this one end up on the 
> release branch as well.

Thanks for flagging that this was missed - definitely a collectively failure 
here for us to recognise this was fixing an important bug in a shipping 
release. We'll try harder to keep in mind the possibility of cherry-picking 
into point releases in the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119541

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119541: [RISCV] Fix RISCVTargetInfo::initFeatureMap, add non-ISA features back after implication

[Alexander Richardson via Phabricator via cfe-commits]

arichardson added a comment.
Herald added subscribers: sunshaoce, StephenFan, shiva0217.
Herald added a project: All.

I just noticed that target features (e.g. -mrelax) are broken in all LLVM 14 
releases due to D113336  . This should have 
been cherry-picked back tot the release branch, but it's too late now. In the 
future please ensure that important fixes such as this one end up on the 
release branch as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119541

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119541: [RISCV] Fix RISCVTargetInfo::initFeatureMap, add non-ISA features back after implication

[Yueh-Ting Chen via Phabricator via cfe-commits]

eopXD marked an inline comment as done.
eopXD added inline comments.



Comment at: clang/test/Driver/riscv-default-features.c:4
+// RV32: "target-features"="+a,+c,+m,+relax,-save-restore"
+// RV64: "target-features"="+64bit,+a,+c,+m,+relax,-save-restore"
+

rogfer01 wrote:
> I think we may be missing are missing a `RUN` line for this case, right?
Created patch D120297, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119541

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119541: [RISCV] Fix RISCVTargetInfo::initFeatureMap, add non-ISA features back after implication

[Roger Ferrer Ibanez via Phabricator via cfe-commits]

rogfer01 added inline comments.



Comment at: clang/test/Driver/riscv-default-features.c:4
+// RV32: "target-features"="+a,+c,+m,+relax,-save-restore"
+// RV64: "target-features"="+64bit,+a,+c,+m,+relax,-save-restore"
+

I think we may be missing are missing a `RUN` line for this case, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119541

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119541: [RISCV] Fix RISCVTargetInfo::initFeatureMap, add non-ISA features back after implication

[Yueh-Ting Chen via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7f51a9e2730e: [RISCV] Fix RISCVTargetInfo::initFeatureMap, 
add non-ISA features back after… (authored by eopXD).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119541

Files:
  clang/lib/Basic/Targets/RISCV.cpp
  clang/test/Driver/riscv-default-features.c


Index: clang/test/Driver/riscv-default-features.c
===
--- /dev/null
+++ clang/test/Driver/riscv-default-features.c
@@ -0,0 +1,9 @@
+// RUN: %clang -target riscv32-unknown-elf -S -emit-llvm %s -o - | FileCheck 
%s -check-prefix=RV32
+
+// RV32: "target-features"="+a,+c,+m,+relax,-save-restore"
+// RV64: "target-features"="+64bit,+a,+c,+m,+relax,-save-restore"
+
+// Dummy function
+int foo(){
+  return  3;
+}
Index: clang/lib/Basic/Targets/RISCV.cpp
===
--- clang/lib/Basic/Targets/RISCV.cpp
+++ clang/lib/Basic/Targets/RISCV.cpp
@@ -232,8 +232,16 @@
 return false;
   }
 
-  return TargetInfo::initFeatureMap(Features, Diags, CPU,
-(*ParseResult)->toFeatureVector());
+  // RISCVISAInfo makes implications for ISA features
+  std::vector ImpliedFeatures = (*ParseResult)->toFeatureVector();
+  // Add non-ISA features like `relax` and `save-restore` back
+  for (std::string Feature : FeaturesVec) {
+if (std::find(begin(ImpliedFeatures), end(ImpliedFeatures), Feature) ==
+end(ImpliedFeatures))
+  ImpliedFeatures.push_back(Feature);
+  }
+
+  return TargetInfo::initFeatureMap(Features, Diags, CPU, ImpliedFeatures);
 }
 
 /// Return true if has this feature, need to sync with handleTargetFeatures.


Index: clang/test/Driver/riscv-default-features.c
===
--- /dev/null
+++ clang/test/Driver/riscv-default-features.c
@@ -0,0 +1,9 @@
+// RUN: %clang -target riscv32-unknown-elf -S -emit-llvm %s -o - | FileCheck %s -check-prefix=RV32
+
+// RV32: "target-features"="+a,+c,+m,+relax,-save-restore"
+// RV64: "target-features"="+64bit,+a,+c,+m,+relax,-save-restore"
+
+// Dummy function
+int foo(){
+  return  3;
+}
Index: clang/lib/Basic/Targets/RISCV.cpp
===
--- clang/lib/Basic/Targets/RISCV.cpp
+++ clang/lib/Basic/Targets/RISCV.cpp
@@ -232,8 +232,16 @@
 return false;
   }
 
-  return TargetInfo::initFeatureMap(Features, Diags, CPU,
-(*ParseResult)->toFeatureVector());
+  // RISCVISAInfo makes implications for ISA features
+  std::vector ImpliedFeatures = (*ParseResult)->toFeatureVector();
+  // Add non-ISA features like `relax` and `save-restore` back
+  for (std::string Feature : FeaturesVec) {
+if (std::find(begin(ImpliedFeatures), end(ImpliedFeatures), Feature) ==
+end(ImpliedFeatures))
+  ImpliedFeatures.push_back(Feature);
+  }
+
+  return TargetInfo::initFeatureMap(Features, Diags, CPU, ImpliedFeatures);
 }
 
 /// Return true if has this feature, need to sync with handleTargetFeatures.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119541: [RISCV] Fix RISCVTargetInfo::initFeatureMap, add non-ISA features back after implication

[JunMa via Phabricator via cfe-commits]

junparser accepted this revision.
junparser added a comment.
This revision is now accepted and ready to land.

LGTM, thanks for the fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119541

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119541: [RISCV] Fix RISCVTargetInfo::initFeatureMap, add non-ISA features back after implication

[Yueh-Ting Chen via Phabricator via cfe-commits]

eopXD updated this revision to Diff 407852.
eopXD added a comment.

Add testcase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119541

Files:
  clang/lib/Basic/Targets/RISCV.cpp
  clang/test/Driver/riscv-default-features.c


Index: clang/test/Driver/riscv-default-features.c
===
--- /dev/null
+++ clang/test/Driver/riscv-default-features.c
@@ -0,0 +1,9 @@
+// RUN: %clang -target riscv32-unknown-elf -S -emit-llvm %s -o - | FileCheck 
%s -check-prefix=RV32
+
+// RV32: "target-features"="+a,+c,+m,+relax,-save-restore"
+// RV64: "target-features"="+64bit,+a,+c,+m,+relax,-save-restore"
+
+// Dummy function
+int foo(){
+  return  3;
+}
Index: clang/lib/Basic/Targets/RISCV.cpp
===
--- clang/lib/Basic/Targets/RISCV.cpp
+++ clang/lib/Basic/Targets/RISCV.cpp
@@ -232,8 +232,16 @@
 return false;
   }
 
-  return TargetInfo::initFeatureMap(Features, Diags, CPU,
-(*ParseResult)->toFeatureVector());
+  // RISCVISAInfo makes implications for ISA features
+  std::vector ImpliedFeatures = (*ParseResult)->toFeatureVector();
+  // Add non-ISA features like `relax` and `save-restore` back
+  for (std::string Feature : FeaturesVec) {
+if (std::find(begin(ImpliedFeatures), end(ImpliedFeatures), Feature) ==
+end(ImpliedFeatures))
+  ImpliedFeatures.push_back(Feature);
+  }
+
+  return TargetInfo::initFeatureMap(Features, Diags, CPU, ImpliedFeatures);
 }
 
 /// Return true if has this feature, need to sync with handleTargetFeatures.


Index: clang/test/Driver/riscv-default-features.c
===
--- /dev/null
+++ clang/test/Driver/riscv-default-features.c
@@ -0,0 +1,9 @@
+// RUN: %clang -target riscv32-unknown-elf -S -emit-llvm %s -o - | FileCheck %s -check-prefix=RV32
+
+// RV32: "target-features"="+a,+c,+m,+relax,-save-restore"
+// RV64: "target-features"="+64bit,+a,+c,+m,+relax,-save-restore"
+
+// Dummy function
+int foo(){
+  return  3;
+}
Index: clang/lib/Basic/Targets/RISCV.cpp
===
--- clang/lib/Basic/Targets/RISCV.cpp
+++ clang/lib/Basic/Targets/RISCV.cpp
@@ -232,8 +232,16 @@
 return false;
   }
 
-  return TargetInfo::initFeatureMap(Features, Diags, CPU,
-(*ParseResult)->toFeatureVector());
+  // RISCVISAInfo makes implications for ISA features
+  std::vector ImpliedFeatures = (*ParseResult)->toFeatureVector();
+  // Add non-ISA features like `relax` and `save-restore` back
+  for (std::string Feature : FeaturesVec) {
+if (std::find(begin(ImpliedFeatures), end(ImpliedFeatures), Feature) ==
+end(ImpliedFeatures))
+  ImpliedFeatures.push_back(Feature);
+  }
+
+  return TargetInfo::initFeatureMap(Features, Diags, CPU, ImpliedFeatures);
 }
 
 /// Return true if has this feature, need to sync with handleTargetFeatures.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D119541: [RISCV] Fix RISCVTargetInfo::initFeatureMap, add non-isa features back after implication

[Yueh-Ting Chen via Phabricator via cfe-commits]

eopXD created this revision.
eopXD added reviewers: junparser, craig.topper, asb, frasercrmck.
Herald added subscribers: VincentWu, luke957, achieveartificialintelligence, 
vkmr, evandro, luismarques, apazos, sameer.abuasal, s.egerton, Jim, benna, 
psnobl, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, 
edward-jones, zzheng, jrtc27, kito-cheng, niosHD, sabuasal, simoncook, 
johnrusso, rbar.
eopXD requested review of this revision.
Herald added subscribers: cfe-commits, pcwang-thead, MaskRay.
Herald added a project: clang.

Previously D113336  makes 
RISCVTargetInfo::initFeatureMap return the results
processed by RISCVISAInfo, which only consists of ISA features and misses
non-ISA features like `relax` and `save-restore`.

This patch fixes the problem.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119541

Files:
  clang/lib/Basic/Targets/RISCV.cpp


Index: clang/lib/Basic/Targets/RISCV.cpp
===
--- clang/lib/Basic/Targets/RISCV.cpp
+++ clang/lib/Basic/Targets/RISCV.cpp
@@ -232,8 +232,16 @@
 return false;
   }
 
-  return TargetInfo::initFeatureMap(Features, Diags, CPU,
-(*ParseResult)->toFeatureVector());
+  // RISCVISAInfo makes implications for ISA features
+  std::vector ImpliedFeatures = (*ParseResult)->toFeatureVector();
+  // Add non-ISA features like `relax` and `save-restore` back
+  for (std::string Feature : FeaturesVec) {
+if (std::find(begin(ImpliedFeatures), end(ImpliedFeatures), Feature) ==
+end(ImpliedFeatures))
+  ImpliedFeatures.push_back(Feature);
+  }
+
+  return TargetInfo::initFeatureMap(Features, Diags, CPU, ImpliedFeatures);
 }
 
 /// Return true if has this feature, need to sync with handleTargetFeatures.


Index: clang/lib/Basic/Targets/RISCV.cpp
===
--- clang/lib/Basic/Targets/RISCV.cpp
+++ clang/lib/Basic/Targets/RISCV.cpp
@@ -232,8 +232,16 @@
 return false;
   }
 
-  return TargetInfo::initFeatureMap(Features, Diags, CPU,
-(*ParseResult)->toFeatureVector());
+  // RISCVISAInfo makes implications for ISA features
+  std::vector ImpliedFeatures = (*ParseResult)->toFeatureVector();
+  // Add non-ISA features like `relax` and `save-restore` back
+  for (std::string Feature : FeaturesVec) {
+if (std::find(begin(ImpliedFeatures), end(ImpliedFeatures), Feature) ==
+end(ImpliedFeatures))
+  ImpliedFeatures.push_back(Feature);
+  }
+
+  return TargetInfo::initFeatureMap(Features, Diags, CPU, ImpliedFeatures);
 }
 
 /// Return true if has this feature, need to sync with handleTargetFeatures.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits