[PATCH] D141768: [11/15][Clang][RISCV][NFC] Remove Policy::PolicyType::Omit

[Yueh-Ting (eop) Chen via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG377d2604e1dd: [11/15][Clang][RISCV][NFC] Remove 
Policy::PolicyType::Omit (authored by eopXD).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141768

Files:
  clang/include/clang/Support/RISCVVIntrinsicUtils.h
  clang/lib/Support/RISCVVIntrinsicUtils.cpp


Index: clang/lib/Support/RISCVVIntrinsicUtils.cpp
===
--- clang/lib/Support/RISCVVIntrinsicUtils.cpp
+++ clang/lib/Support/RISCVVIntrinsicUtils.cpp
@@ -969,10 +969,10 @@
 llvm::SmallVector
 RVVIntrinsic::getSupportedUnMaskedPolicies(bool HasTailPolicy,
bool HasMaskPolicy) {
-  return {Policy(Policy::PolicyType::Undisturbed, Policy::PolicyType::Omit,
- HasTailPolicy, HasMaskPolicy), // TU
-  Policy(Policy::PolicyType::Agnostic, Policy::PolicyType::Omit,
- HasTailPolicy, HasMaskPolicy)}; // TA
+  return {
+  Policy(Policy::PolicyType::Undisturbed, HasTailPolicy,
+ HasMaskPolicy),   // 
TU
+  Policy(Policy::PolicyType::Agnostic, HasTailPolicy, HasMaskPolicy)}; // 
TA
 }
 
 llvm::SmallVector
@@ -1034,28 +1034,33 @@
 BuiltinName += "_ta";
 }
   } else {
-if (PolicyAttrs.isTUMAPolicy() && !PolicyAttrs.hasMaskPolicy())
-  appendPolicySuffix("_tum");
-else if (PolicyAttrs.isTAMAPolicy() && !PolicyAttrs.hasMaskPolicy())
-  appendPolicySuffix("_tam");
-else if (PolicyAttrs.isMUPolicy() && !PolicyAttrs.hasTailPolicy())
-  appendPolicySuffix("_mu");
-else if (PolicyAttrs.isMAPolicy() && !PolicyAttrs.hasTailPolicy())
-  appendPolicySuffix("_ma");
-else if (PolicyAttrs.isTUMUPolicy())
-  appendPolicySuffix("_tumu");
-else if (PolicyAttrs.isTAMUPolicy())
-  appendPolicySuffix("_tamu");
-else if (PolicyAttrs.isTUMAPolicy())
-  appendPolicySuffix("_tuma");
-else if (PolicyAttrs.isTAMAPolicy())
-  appendPolicySuffix("_tama");
-else if (PolicyAttrs.isTUPolicy() && !IsMasked)
-  appendPolicySuffix("_tu");
-else if (PolicyAttrs.isTAPolicy() && !IsMasked)
-  appendPolicySuffix("_ta");
-else
-  llvm_unreachable("Unhandled policy condition");
+if (IsMasked) {
+  if (PolicyAttrs.isTUMAPolicy() && !PolicyAttrs.hasMaskPolicy())
+appendPolicySuffix("_tum");
+  else if (PolicyAttrs.isTAMAPolicy() && !PolicyAttrs.hasMaskPolicy())
+appendPolicySuffix("_tam");
+  else if (PolicyAttrs.isMUPolicy() && !PolicyAttrs.hasTailPolicy())
+appendPolicySuffix("_mu");
+  else if (PolicyAttrs.isMAPolicy() && !PolicyAttrs.hasTailPolicy())
+appendPolicySuffix("_ma");
+  else if (PolicyAttrs.isTUMUPolicy())
+appendPolicySuffix("_tumu");
+  else if (PolicyAttrs.isTAMUPolicy())
+appendPolicySuffix("_tamu");
+  else if (PolicyAttrs.isTUMAPolicy())
+appendPolicySuffix("_tuma");
+  else if (PolicyAttrs.isTAMAPolicy())
+appendPolicySuffix("_tama");
+  else
+llvm_unreachable("Unhandled policy condition");
+} else {
+  if (PolicyAttrs.isTUPolicy())
+appendPolicySuffix("_tu");
+  else if (PolicyAttrs.isTAPolicy())
+appendPolicySuffix("_ta");
+  else
+llvm_unreachable("Unhandled policy condition");
+}
   }
 }
 
Index: clang/include/clang/Support/RISCVVIntrinsicUtils.h
===
--- clang/include/clang/Support/RISCVVIntrinsicUtils.h
+++ clang/include/clang/Support/RISCVVIntrinsicUtils.h
@@ -97,7 +97,6 @@
   enum PolicyType {
 Undisturbed,
 Agnostic,
-Omit, // No policy required.
   };
   PolicyType TailPolicy = Agnostic;
   PolicyType MaskPolicy = Undisturbed;
@@ -105,6 +104,9 @@
   Policy(bool HasTailPolicy, bool HasMaskPolicy)
   : IsUnspecified(true), HasTailPolicy(HasTailPolicy),
 HasMaskPolicy(HasMaskPolicy) {}
+  Policy(PolicyType TailPolicy, bool HasTailPolicy, bool HasMaskPolicy)
+  : TailPolicy(TailPolicy), HasTailPolicy(HasTailPolicy),
+HasMaskPolicy(HasMaskPolicy) {}
   Policy(PolicyType TailPolicy, PolicyType MaskPolicy, bool HasTailPolicy,
  bool HasMaskPolicy)
   : TailPolicy(TailPolicy), MaskPolicy(MaskPolicy),


Index: clang/lib/Support/RISCVVIntrinsicUtils.cpp
===
--- clang/lib/Support/RISCVVIntrinsicUtils.cpp
+++ clang/lib/Support/RISCVVIntrinsicUtils.cpp
@@ -969,10 +969,10 @@
 llvm::SmallVector
 RVVIntrinsic::getSupportedUnMaskedPolicies(bool HasTailPolicy,
bool HasMaskPolicy) {
-  return {Policy(Policy::PolicyType::Undisturbed, Policy::PolicyType::Omit,
- HasTailPolicy, 

[PATCH] D141768: [11/15][Clang][RISCV][NFC] Remove Policy::PolicyType::Omit

[Yueh-Ting (eop) Chen via Phabricator via cfe-commits]

eopXD updated this revision to Diff 491805.
eopXD edited the summary of this revision.
eopXD added a comment.

Rebase to latest main.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141768

Files:
  clang/include/clang/Support/RISCVVIntrinsicUtils.h
  clang/lib/Support/RISCVVIntrinsicUtils.cpp


Index: clang/lib/Support/RISCVVIntrinsicUtils.cpp
===
--- clang/lib/Support/RISCVVIntrinsicUtils.cpp
+++ clang/lib/Support/RISCVVIntrinsicUtils.cpp
@@ -969,10 +969,10 @@
 llvm::SmallVector
 RVVIntrinsic::getSupportedUnMaskedPolicies(bool HasTailPolicy,
bool HasMaskPolicy) {
-  return {Policy(Policy::PolicyType::Undisturbed, Policy::PolicyType::Omit,
- HasTailPolicy, HasMaskPolicy), // TU
-  Policy(Policy::PolicyType::Agnostic, Policy::PolicyType::Omit,
- HasTailPolicy, HasMaskPolicy)}; // TA
+  return {
+  Policy(Policy::PolicyType::Undisturbed, HasTailPolicy,
+ HasMaskPolicy),   // 
TU
+  Policy(Policy::PolicyType::Agnostic, HasTailPolicy, HasMaskPolicy)}; // 
TA
 }
 
 llvm::SmallVector
@@ -1034,28 +1034,33 @@
 BuiltinName += "_ta";
 }
   } else {
-if (PolicyAttrs.isTUMAPolicy() && !PolicyAttrs.hasMaskPolicy())
-  appendPolicySuffix("_tum");
-else if (PolicyAttrs.isTAMAPolicy() && !PolicyAttrs.hasMaskPolicy())
-  appendPolicySuffix("_tam");
-else if (PolicyAttrs.isMUPolicy() && !PolicyAttrs.hasTailPolicy())
-  appendPolicySuffix("_mu");
-else if (PolicyAttrs.isMAPolicy() && !PolicyAttrs.hasTailPolicy())
-  appendPolicySuffix("_ma");
-else if (PolicyAttrs.isTUMUPolicy())
-  appendPolicySuffix("_tumu");
-else if (PolicyAttrs.isTAMUPolicy())
-  appendPolicySuffix("_tamu");
-else if (PolicyAttrs.isTUMAPolicy())
-  appendPolicySuffix("_tuma");
-else if (PolicyAttrs.isTAMAPolicy())
-  appendPolicySuffix("_tama");
-else if (PolicyAttrs.isTUPolicy() && !IsMasked)
-  appendPolicySuffix("_tu");
-else if (PolicyAttrs.isTAPolicy() && !IsMasked)
-  appendPolicySuffix("_ta");
-else
-  llvm_unreachable("Unhandled policy condition");
+if (IsMasked) {
+  if (PolicyAttrs.isTUMAPolicy() && !PolicyAttrs.hasMaskPolicy())
+appendPolicySuffix("_tum");
+  else if (PolicyAttrs.isTAMAPolicy() && !PolicyAttrs.hasMaskPolicy())
+appendPolicySuffix("_tam");
+  else if (PolicyAttrs.isMUPolicy() && !PolicyAttrs.hasTailPolicy())
+appendPolicySuffix("_mu");
+  else if (PolicyAttrs.isMAPolicy() && !PolicyAttrs.hasTailPolicy())
+appendPolicySuffix("_ma");
+  else if (PolicyAttrs.isTUMUPolicy())
+appendPolicySuffix("_tumu");
+  else if (PolicyAttrs.isTAMUPolicy())
+appendPolicySuffix("_tamu");
+  else if (PolicyAttrs.isTUMAPolicy())
+appendPolicySuffix("_tuma");
+  else if (PolicyAttrs.isTAMAPolicy())
+appendPolicySuffix("_tama");
+  else
+llvm_unreachable("Unhandled policy condition");
+} else {
+  if (PolicyAttrs.isTUPolicy())
+appendPolicySuffix("_tu");
+  else if (PolicyAttrs.isTAPolicy())
+appendPolicySuffix("_ta");
+  else
+llvm_unreachable("Unhandled policy condition");
+}
   }
 }
 
Index: clang/include/clang/Support/RISCVVIntrinsicUtils.h
===
--- clang/include/clang/Support/RISCVVIntrinsicUtils.h
+++ clang/include/clang/Support/RISCVVIntrinsicUtils.h
@@ -97,7 +97,6 @@
   enum PolicyType {
 Undisturbed,
 Agnostic,
-Omit, // No policy required.
   };
   PolicyType TailPolicy = Agnostic;
   PolicyType MaskPolicy = Undisturbed;
@@ -105,6 +104,9 @@
   Policy(bool HasTailPolicy, bool HasMaskPolicy)
   : IsUnspecified(true), HasTailPolicy(HasTailPolicy),
 HasMaskPolicy(HasMaskPolicy) {}
+  Policy(PolicyType TailPolicy, bool HasTailPolicy, bool HasMaskPolicy)
+  : TailPolicy(TailPolicy), HasTailPolicy(HasTailPolicy),
+HasMaskPolicy(HasMaskPolicy) {}
   Policy(PolicyType TailPolicy, PolicyType MaskPolicy, bool HasTailPolicy,
  bool HasMaskPolicy)
   : TailPolicy(TailPolicy), MaskPolicy(MaskPolicy),


Index: clang/lib/Support/RISCVVIntrinsicUtils.cpp
===
--- clang/lib/Support/RISCVVIntrinsicUtils.cpp
+++ clang/lib/Support/RISCVVIntrinsicUtils.cpp
@@ -969,10 +969,10 @@
 llvm::SmallVector
 RVVIntrinsic::getSupportedUnMaskedPolicies(bool HasTailPolicy,
bool HasMaskPolicy) {
-  return {Policy(Policy::PolicyType::Undisturbed, Policy::PolicyType::Omit,
- HasTailPolicy, HasMaskPolicy), // TU
-  Policy(Policy::PolicyType::Agnostic, Policy::PolicyType::Omit,
- 

[PATCH] D141768: [11/15][Clang][RISCV][NFC] Remove Policy::PolicyType::Omit

[Yueh-Ting (eop) Chen via Phabricator via cfe-commits]

eopXD added inline comments.



Comment at: clang/lib/Support/RISCVVIntrinsicUtils.cpp:1032
+if (IsMasked) {
+  if (PolicyAttrs.isTUMAPolicy() && !HasMaskPolicy)
+appendPolicySuffix("_tum");

craig.topper wrote:
> If you hadn't just removed Omit, it somewhat feels like Omit should be part 
> of Policy and this code to pick the policy suffix string should be inside the 
> Policy class. But I'll defer to your judgement.
Good idea, I will encapsulate logics into `Policy`. Considering the next 
patch-set, to simplify the policy suffixes, `IsUnspecified` can be further 
removed. Eventually we can have something like:

```
void RVVIntrinsic::updateNamesAndPolicy(bool IsMasked, bool HasPolicy,
std::string ,
std::string ,
std::string ,
Policy ) {
  std::string PolicySuffix =
PolicyAttrs.getPolicySuffix(IsMasked, HasPolicy);
  Name += PolicySuffix;
  BuiltinName += PolicySuffix;
  OverloadedName += PolicySuffix;
}
```

I will create a separate revision so we won't be distracted towards the goal of 
this patch-set.

---

On the other hand, here is my thought process when considering the removal of 
`Omit` as a legit refactoring.

Logics to deal with `Omit` occurs under here (`updateNamesAndPolicy`), 
`computeBuiltinTypes`, and `getPolicyAttrs`. The `Omit` state specified to a 
`Policy` implies it has to be corrected to something specific (either Agnostic 
or Undisturbed) before calling `getPolicyAttrs`, which will set the values in 
`riscv_vector_cg` (called under `RISCVVEmitter.cpp`. This blocks the 
possibility of setting `TailPolicy`, `MaskPolicy` as constant members.

I picked `HasTailPolicy` and `HasMaskPolicy` for the predicates because it was 
exactly how `riscv_vector.td` defined the intrinsics. They are necessary as 
conditions now with the current policy suffix naming rules. With the next 
patch-set, `HasTailPolicy` and `HasMaskPolicy` will only be called by 
`getSupportedUnMaskedPolicies` and `getSupportedMaskedPolicies`. This is 
because the policy suffix logics here can be simplified to something like: 
(this does not contradict with the encapsulating topic above)

```
if (PolicyAttrs.isUnspecified()) {
  if (IsMasked) {
Name += "_m";
if (HasPolicy)
  BuiltinName += "_tama";
else
  BuiltinName += "_m";
  } else {
if (HasPolicy)
  BuiltinName += "_ta";
  }
} else {
  if (IsMasked) {
if (PolicyAttrs.isTAMUPolicy())
  appendPolicySuffix("_mu")
else if (PolicyAttrs.isTUMAPolicy())
  appendPolicySuffix("_tum")
else if (PolicyAttrs.isTUMUPolicy())
  appendPolicySuffix("_tumu")
else
  llvm_unreachable("Unhandled policy condition");
  } else {
if (PolicyAttrs.isTUPolicy())
  appendPolicySuffix("_tu");
else
  llvm_unreachable("Unhandled policy condition");
  }
}
```  




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141768

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

[PATCH] D141768: [11/15][Clang][RISCV][NFC] Remove Policy::PolicyType::Omit

[Craig Topper via Phabricator via cfe-commits]

craig.topper accepted this revision.
craig.topper added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141768

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

[PATCH] D141768: [11/15][Clang][RISCV][NFC] Remove Policy::PolicyType::Omit

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: clang/lib/Support/RISCVVIntrinsicUtils.cpp:1032
+if (IsMasked) {
+  if (PolicyAttrs.isTUMAPolicy() && !HasMaskPolicy)
+appendPolicySuffix("_tum");

If you hadn't just removed Omit, it somewhat feels like Omit should be part of 
Policy and this code to pick the policy suffix string should be inside the 
Policy class. But I'll defer to your judgement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141768

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

[PATCH] D141768: [11/15][Clang][RISCV][NFC] Remove Policy::PolicyType::Omit

[Kito Cheng via Phabricator via cfe-commits]

kito-cheng accepted this revision.
kito-cheng added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: luke.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141768

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