[mips] [IAS] Restore STI.FeatureBits in .set pop.

[tomatabacu]

Summary

Only restoring AvailableFeatures is not enough and will lead to buggy behaviour.
For example, if we have a feature enabled and we ".set pop", the next time we try
to ".set" that feature nothing will happen because the "!(STI.getFeatureBits()[Feature])"
check will be false, because we didn't restore STI.FeatureBits.

In order to fix this, we need to make MipsAssemblerOptions remember the STI.FeatureBits
instead of the AvailableFeatures and then regenerate AvailableFeatures each time we ".set pop".
This is because, AFAIK, there is no way to convert from AvailableFeatures back to STI.FeatureBits,
but the reverse is possible by using ComputeAvailableFeatures(STI.FeatureBits).

I also moved the updating of AssemblerOptions inside the "if" statement in
setFeatureBits() and clearFeatureBits(), as there is no reason to update if
nothing changes.

[tomatabacu]

Ping.

[tomatabacu]

Ping.

[dsanders]

LGTM with an extended test case to cover '.set pop' enabling a feature that was indirectly disabled inside a .set push/pop region.

test/MC/Mips/set-push-pop-directives.s

70

We ought to add a test like:

.set msa
.set push
.set mips32 # MSA not available in MIPS32
addvi.b $w1, $w2, 1 # CHECK: :[[@LINE-1]]:5: error: instruction requires a CPU feature not currently enabled
.set pop
addvi.b $w1, $w2, 1 # CHECK-NOT: :[[@LINE-1]]:5: error: instruction requires a CPU feature not currently enabled

[tomatabacu]

test/MC/Mips/set-push-pop-directives.s

70

In GAS (and the IAS) ".set mips32" does not turn off MSA.
So there are no errors in this case, but GAS does emit warnings for inappropriate option combos (including MIPS32 and MSA).

These warnings are not implemented in the IAS at the moment.
They can be added in a separate patch, so I don't think they should block this patch.

[dsanders]

test/MC/Mips/set-push-pop-directives.s

70

MSA was only used as a convenient example. There should be other features bits that can be used to test that indirectly disabled features become enabled again after a .set pop.

In GAS (and the IAS) ".set mips32" does not turn off MSA.
So there are no errors in this case, but GAS does emit warnings for inappropriate option combos (including MIPS32 and MSA).

It should emit a diagnostic of some kind. It's not valid to use MSA with MIPS32 or earlier.

Note that the MSA feature is technically still enabled after the '.set mips32' but using MSA instructions is invalid (and should emit errors or warnings) until the '.set pop' restores the ISA to MIPS32r2.

[tomatabacu]

Added requested test case.
Rebased.
Had to make additional changes because of rL238192.

[tomatabacu]

Added Michael Kuperstein as a reviewer because of changes related to his recent STI.FeatureBits work.

[dsanders]

LGTM

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

46

Nit: Space goes before the '&' rather than after.

74

Nit: Here too.

[tomatabacu]

Addressed spacing nits.

Waiting for Michael Kuperstein's approval.

[dsanders]

Waiting for Michael Kuperstein's approval.

I don't think there's a need for Michael to explicitly LGTM this patch. One LGTM is usually sufficient and the common-code part of the change is just a formatting nit and adding a const.

[mkuper]

Sorry for the latency, I was away for a few days.
Anyway, the target-independent part LGTM. The const really should have been there to begin with, thanks! :-)

include/llvm/MC/MCSubtargetInfo.h

public:
   /// getFeatureBits - Return the feature bits.
   ///
   const FeatureBitset& getFeatureBits() const {
     return FeatureBits;
   }
 
   /// setFeatureBits - Set the feature bits.
   ///
-  void setFeatureBits(FeatureBitset& FeatureBits_) { FeatureBits = FeatureBits_; }
+  void setFeatureBits(const FeatureBitset &FeatureBits_) {
+    FeatureBits = FeatureBits_;
+  }
 
   /// InitMCProcessorInfo - Set or change the CPU (optionally supplemented with
   /// feature string). Recompute feature bits and scheduling model.
   void InitMCProcessorInfo(StringRef CPU, StringRef FS);
 
   /// InitCPUSchedModel - Recompute scheduling model based on CPU.
   void InitCPUSchedModel(StringRef CPU);
 

lib/Target/Mips/AsmParser/MipsAsmParser.cpp

 
 namespace llvm {
 class MCInstrInfo;
 }
 
 namespace {
 class MipsAssemblerOptions {
 public:
-  MipsAssemblerOptions(uint64_t Features_) : 
+  MipsAssemblerOptions(const FeatureBitset &Features_) :
     ATReg(1), Reorder(true), Macro(true), Features(Features_) {}
 
   MipsAssemblerOptions(const MipsAssemblerOptions *Opts) {
     ATReg = Opts->getATRegIndex();
     Reorder = Opts->isReorder();
     Macro = Opts->isMacro();
     Features = Opts->getFeatures();
   }
public:
   bool isReorder() const { return Reorder; }
   void setReorder() { Reorder = true; }
   void setNoReorder() { Reorder = false; }
 
   bool isMacro() const { return Macro; }
   void setMacro() { Macro = true; }
   void setNoMacro() { Macro = false; }
 
-  uint64_t getFeatures() const { return Features; }
-  void setFeatures(uint64_t Features_) { Features = Features_; }
+  const FeatureBitset &getFeatures() const { return Features; }
+  void setFeatures(const FeatureBitset &Features_) { Features = Features_; }
 
   // Set of features that are either architecture features or referenced
   // by them (e.g.: FeatureNaN2008 implied by FeatureMips32r6).
   // The full table can be found in MipsGenSubtargetInfo.inc (MipsFeatureKV[]).
   // The reason we need this mask is explained in the selectArch function.
   // FIXME: Ideally we would like TableGen to generate this information.
   static const FeatureBitset AllArchRelatedMask;
 
 private:
   unsigned ATReg;
   bool Reorder;
   bool Macro;
-  uint64_t Features;
+  FeatureBitset Features;
 };
 }
 
 const FeatureBitset MipsAssemblerOptions::AllArchRelatedMask = {
     Mips::FeatureMips1, Mips::FeatureMips2, Mips::FeatureMips3,
     Mips::FeatureMips3_32, Mips::FeatureMips3_32r2, Mips::FeatureMips4,
     Mips::FeatureMips4_32, Mips::FeatureMips4_32r2, Mips::FeatureMips5,
     Mips::FeatureMips5_32r2, Mips::FeatureMips32, Mips::FeatureMips32r2,
#include "MipsGenAsmMatcher.inc"
   // FeatureMipsGP64 | FeatureMips1)
   // Clearing Mips3 is equivalent to clear (FeatureMips3 | FeatureMips4).
   void selectArch(StringRef ArchFeature) {
     FeatureBitset FeatureBits = STI.getFeatureBits();
     FeatureBits &= ~MipsAssemblerOptions::AllArchRelatedMask;
     STI.setFeatureBits(FeatureBits);
     setAvailableFeatures(
         ComputeAvailableFeatures(STI.ToggleFeature(ArchFeature)));
-    AssemblerOptions.back()->setFeatures(getAvailableFeatures());
+    AssemblerOptions.back()->setFeatures(STI.getFeatureBits());
   }
 
   void setFeatureBits(uint64_t Feature, StringRef FeatureString) {
     if (!(STI.getFeatureBits()[Feature])) {
       setAvailableFeatures(
           ComputeAvailableFeatures(STI.ToggleFeature(FeatureString)));
+      AssemblerOptions.back()->setFeatures(STI.getFeatureBits());
     }
-    AssemblerOptions.back()->setFeatures(getAvailableFeatures());
   }
 
   void clearFeatureBits(uint64_t Feature, StringRef FeatureString) {
     if (STI.getFeatureBits()[Feature]) {
       setAvailableFeatures(
           ComputeAvailableFeatures(STI.ToggleFeature(FeatureString)));
+      AssemblerOptions.back()->setFeatures(STI.getFeatureBits());
     }
-    AssemblerOptions.back()->setFeatures(getAvailableFeatures());
   }
 
 public:
   enum MipsMatchResultTy {
     Match_RequiresDifferentSrcAndDst = FIRST_TARGET_MATCH_RESULT_TY
 #define GET_OPERAND_DIAGNOSTIC_TYPES
 #include "MipsGenAsmMatcher.inc"
 #undef GET_OPERAND_DIAGNOSTIC_TYPES
MipsAsmParser(MCSubtargetInfo &sti, MCAsmParser &parser,
 
     parser.addAliasForDirective(".asciiz", ".asciz");
 
     // Initialize the set of available features.
     setAvailableFeatures(ComputeAvailableFeatures(STI.getFeatureBits()));
     
     // Remember the initial assembler options. The user can not modify these.
     AssemblerOptions.push_back(
-                     make_unique<MipsAssemblerOptions>(getAvailableFeatures()));
+                     make_unique<MipsAssemblerOptions>(STI.getFeatureBits()));
     
     // Create an assembler options environment for the user to modify.
     AssemblerOptions.push_back(
-                     make_unique<MipsAssemblerOptions>(getAvailableFeatures()));
+                     make_unique<MipsAssemblerOptions>(STI.getFeatureBits()));
 
     getTargetStreamer().updateABIInfo(*this);
 
     if (!isABI_O32() && !useOddSPReg() != 0)
       report_fatal_error("-mno-odd-spreg requires the O32 ABI");
 
     CurrentFn = nullptr;
   }
if (getLexer().isNot(AsmToken::EndOfStatement))
     return reportParseError("unexpected token, expected end of statement");
 
   // Always keep an element on the options "stack" to prevent the user
   // from changing the initial options. This is how we remember them.
   if (AssemblerOptions.size() == 2)
     return reportParseError(Loc, ".set pop with no .set push");
 
   AssemblerOptions.pop_back();
-  setAvailableFeatures(AssemblerOptions.back()->getFeatures());
+  setAvailableFeatures(
+      ComputeAvailableFeatures(AssemblerOptions.back()->getFeatures()));
+  STI.setFeatureBits(AssemblerOptions.back()->getFeatures());
 
   getTargetStreamer().emitDirectiveSetPop();
   return false;
 }
 
 bool MipsAsmParser::parseSetPushDirective() {
   MCAsmParser &Parser = getParser();
   Parser.Lex();

 
 bool MipsAsmParser::parseSetMips0Directive() {
   MCAsmParser &Parser = getParser();
   Parser.Lex();
   if (getLexer().isNot(AsmToken::EndOfStatement))
     return reportParseError("unexpected token, expected end of statement");
 
   // Reset assembler options to their initial values.
-  setAvailableFeatures(AssemblerOptions.front()->getFeatures());
+  setAvailableFeatures(
+      ComputeAvailableFeatures(AssemblerOptions.front()->getFeatures()));
+  STI.setFeatureBits(AssemblerOptions.front()->getFeatures());
   AssemblerOptions.back()->setFeatures(AssemblerOptions.front()->getFeatures());
 
   getTargetStreamer().emitDirectiveSetMips0();
   return false;
 }
 
 bool MipsAsmParser::parseSetArchDirective() {
   MCAsmParser &Parser = getParser();

test/MC/Mips/set-push-pop-directives-bad.s

 # RUN: not llvm-mc %s -triple=mipsel-unknown-linux -mcpu=mips32r2 2>%t1
 # RUN:   FileCheck %s < %t1
 
         .text
         .set pop
 # CHECK: :[[@LINE-1]]:14: error: .set pop with no .set push
         .set push
         .set pop
         .set pop
 # CHECK: :[[@LINE-1]]:14: error: .set pop with no .set push
         .set push foo
 # CHECK: :[[@LINE-1]]:19: error: unexpected token, expected end of statement
         .set pop bar
 # CHECK: :[[@LINE-1]]:18: error: unexpected token, expected end of statement
+
+        .set hardfloat
+        .set push
+        .set softfloat
+        add.s $f2, $f2, $f2
+# CHECK: :[[@LINE-1]]:9: error: instruction requires a CPU feature not currently enabled
+        .set pop
+        add.s $f2, $f2, $f2
+# CHECK-NOT: :[[@LINE-1]]:9: error: instruction requires a CPU feature not currently enabled

test/MC/Mips/set-push-pop-directives.s

 
 # CHECK:  .set pop
 # CHECK:  lui      $1, 1
 # CHECK:  addu     $1, $1, $1
 # CHECK:  lw       $1, 0($1)
 # CHECK:  b        1336
 # CHECK:  nop
 # CHECK:  addvi.b  $w15, $w13, 18
+
+    .set push
+    .set dsp
+    lbux    $7, $10($11)
+    .set pop
+
+    .set push
+    .set dsp
+    lbux    $7, $10($11)
+# CHECK-NOT: :[[@LINE-1]]:5: error: instruction requires a CPU feature not currently enabled
+    .set pop
+
+    .set push
+    .set dsp
+    lbux    $7, $10($11)
+# CHECK-NOT: :[[@LINE-1]]:5: error: instruction requires a CPU feature not currently enabled
+    .set pop