[PATCH] D50977: [TableGen] Examine entire subreg compositions to detect ambiguity

[Krzysztof Parzyszek via Phabricator via llvm-commits]

kparzysz marked 2 inline comments as done.
kparzysz added inline comments.


================
Comment at: test/TableGen/ambiguous-composition.td:16
+let Namespace = "Test" in {
+  def subreg_l32  : SubRegIndex<32, 32>;
+  def subreg_h32  : SubRegIndex<32, 32>;
----------------
bjope wrote:
> I assume this should be
>   def subreg_l32  : SubRegIndex<32, 0>;
> or 
>   def subreg_l32  : SubRegIndex<32>;
> 
> (I think the tested scenario still should be valid, but we avoid the possible confusion with subreg_h32 and subreg_l32 having the same definition)
The numbers don't matter at all, except for debug information, but you're right---they should be correct regardless.


================
Comment at: test/TableGen/ambiguous-composition.td:37
+// 
+// For the register V0Q, subreg_hh32(V0Q) = subreg_h32(V0Q) = F0S, which
+// would be enough to trigger the warning about ambiguous composition.
----------------
bjope wrote:
> Isn't this ambiguity still making the register/subregister definitions for SystemZ a little bit dubious. I do not have an example where it actually is causing any problems, but it feels weird.
> Anyway, if there is no better way to describe the SystemZ registers I guess we have to live with this oddity.
With the current handling of subregisters by TableGen there is no way to completely avoid partially overlapping compositions.  If we disallow partial overlaps, we get the warning that people have been complaining about (i.e. more or less the current state of things).  If we allow them, we end up with the undiagnosed subreg_ll32 issue (discussed earlier).
This solution is ugly and it is a hack to avoid the false-positive warning, but it should work as long as subregister composition is done consistently in the same manner on the overlapping parts.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D50977