[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 &Name,
                                        std::string &BuiltinName,
                                        std::string &OverloadedName,
                                        Policy &PolicyAttrs) {
  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