[GlobalISel] Fix known bits for G_ASSERT_ALIGN.

[aemerson]

Summary

I don't know what was going on originally with these tests. It seems reasonable to have the immediate be the same byte alignment unit as the IR, in which case we need to take the log2 in order to set the right number of low bits.

This fixes a miscompile in chromium.

[paquette]

LGTM

[thakis]

This breaks building on platforms where int64_t is long: http://45.33.8.238/linux/87097/step_4.txt

[arsenm]

llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp

42

This might have been a misuse

475

I thought 0 would be rejected by the verifier. This shouldn’t have been produced

[doru1004]

I am getting this error with the latest main:

/home/dobercea/upstream/llvm-project/llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp: In member function ‘llvm::Align llvm::GISelKnownBits::computeKnownAlignment(llvm::Register, unsigned int)’:
/home/dobercea/upstream/llvm-project/llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp:42:58: error: no matching function for call to ‘max(long long int, int64_t)’
   42 |     return Align(std::max(1LL, MI->getOperand(2).getImm()));
        |                                                        ^

[aemerson]

In D134380#3806943, @doru1004 wrote:

I am getting this error with the latest main:

/home/dobercea/upstream/llvm-project/llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp: In member function ‘llvm::Align llvm::GISelKnownBits::computeKnownAlignment(llvm::Register, unsigned int)’:
/home/dobercea/upstream/llvm-project/llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp:42:58: error: no matching function for call to ‘max(long long int, int64_t)’
   42 |     return Align(std::max(1LL, MI->getOperand(2).getImm()));
        |                                                        ^

Sorry about that, someone fixed this in 2d975f1efe6cdb4f6303cfe33a3a06cbe02e755a

[aemerson]

llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp

475

Ok I'll look into adding a verifier check and removing this handling for 0.

[arsenm]

llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp

475

Actually this is a different change than I thought. The immediate was supposed to be directly interpreted as the log2 value. Somewhere must be producing it with the raw alignment instead of the correct log2 value

475

0 should still be invalid though

[aemerson]

llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp

475

Why bother with encoding it as a bit mask when other places use the number of bytes?

[arsenm]

llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp

475

It's not a bitmask it's a number of bits. The only use is here to set a number of bits

[aemerson]

llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp

475

Right ok but same point, why deviate from the semantics of "alignment" at other IR levels?

[arsenm]

llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp

475

I guess it doesn't really matter but everywhere needs to be consistent. I thought a value you can directly use would be more convenient

llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp

 
 Align GISelKnownBits::computeKnownAlignment(Register R, unsigned Depth) {
   const MachineInstr *MI = MRI.getVRegDef(R);
   switch (MI->getOpcode()) {
   case TargetOpcode::COPY:
     return computeKnownAlignment(MI->getOperand(1).getReg(), Depth);
   case TargetOpcode::G_ASSERT_ALIGN: {
     // TODO: Min with source
-    int64_t LogAlign = MI->getOperand(2).getImm();
-    return Align(1ull << LogAlign);
+    return Align(std::max(1LL, MI->getOperand(2).getImm()));
   }
   case TargetOpcode::G_FRAME_INDEX: {
     int FrameIdx = MI->getOperand(1).getIndex();
     return MF.getFrameInfo().getObjectAlign(FrameIdx);
   }
   case TargetOpcode::G_INTRINSIC:
   case TargetOpcode::G_INTRINSIC_W_SIDE_EFFECTS:
   default:
case TargetOpcode::G_TRUNC: {
     Known = Known.zextOrTrunc(SrcBitWidth);
     computeKnownBitsImpl(SrcReg, Known, DemandedElts, Depth + 1);
     Known = Known.zextOrTrunc(BitWidth);
     if (BitWidth > SrcBitWidth)
       Known.Zero.setBitsFrom(SrcBitWidth);
     break;
   }
   case TargetOpcode::G_ASSERT_ALIGN: {
-    int64_t LogOfAlign = MI.getOperand(2).getImm();
-    if (LogOfAlign == 0)
+    int64_t Align = MI.getOperand(2).getImm();
+    if (Align == 0)
       break;
+    int64_t LogOfAlign = Log2_64(Align);
 
     // TODO: Should use maximum with source
     // If a node is guaranteed to be aligned, set low zero bits accordingly as
     // well as clearing one bits.
     Known.Zero.setLowBits(LogOfAlign);
     Known.One.clearLowBits(LogOfAlign);
     break;
   }

llvm/unittests/CodeGen/GlobalISel/KnownBitsTest.cpp

TEST_F(AMDGPUGISelMITest, TestKnownBitsAssertAlign) {
    %copy_assert_align0:_(s64) = COPY %assert_align0
 
    %assert_align1:_(s64) = G_ASSERT_ALIGN %val, 1
    %copy_assert_align1:_(s64) = COPY %assert_align1
 
    %assert_align2:_(s64) = G_ASSERT_ALIGN %val, 2
    %copy_assert_align2:_(s64) = COPY %assert_align2
 
-   %assert_align3:_(s64) = G_ASSERT_ALIGN %val, 3
-   %copy_assert_align3:_(s64) = COPY %assert_align3
+   %assert_align4:_(s64) = G_ASSERT_ALIGN %val, 4
+   %copy_assert_align4:_(s64) = COPY %assert_align4
 
    %assert_align8:_(s64) = G_ASSERT_ALIGN %val, 8
    %copy_assert_align8:_(s64) = COPY %assert_align8
 
-   %assert_maxalign:_(s64) = G_ASSERT_ALIGN %val, 30
-   %copy_assert_maxalign:_(s64) = COPY %assert_maxalign
-
-   %assert_ptr_align5:_(p1) = G_ASSERT_ALIGN %ptrval, 5
-   %copy_assert_ptr_align5:_(p1) = COPY %assert_ptr_align5
+   %assert_align16:_(s64) = G_ASSERT_ALIGN %val, 16
+   %copy_assert_maxalign:_(s64) = COPY %assert_align16
 )MIR";
   setUp(MIRString);
   if (!TM)
     return;
   GISelKnownBits Info(*MF);
 
   KnownBits Res;
   auto GetKB = [&](unsigned Idx) {
     Register CopyReg = Copies[Idx];
     auto *Copy = MRI->getVRegDef(CopyReg);
     return Info.getKnownBits(Copy->getOperand(1).getReg());
   };
 
   auto CheckBits = [&](unsigned NumBits, unsigned Idx) {
     Res = GetKB(Idx);
     EXPECT_EQ(64u, Res.getBitWidth());
-    EXPECT_EQ(NumBits, Res.Zero.countTrailingOnes());
+    EXPECT_EQ(NumBits - 1, Res.Zero.countTrailingOnes());
     EXPECT_EQ(64u, Res.One.countTrailingZeros());
-    EXPECT_EQ(Align(1ull << NumBits), Info.computeKnownAlignment(Copies[Idx]));
+    EXPECT_EQ(Align(1ull << (NumBits - 1)), Info.computeKnownAlignment(Copies[Idx]));
   };
 
-  CheckBits(0, Copies.size() - 7);
-  CheckBits(1, Copies.size() - 6);
-  CheckBits(2, Copies.size() - 5);
-  CheckBits(3, Copies.size() - 4);
-  CheckBits(8, Copies.size() - 3);
-  CheckBits(30, Copies.size() - 2);
-  CheckBits(5, Copies.size() - 1);
+  const unsigned NumSetupCopies = 5;
+  // Check zero align specially.
+  Res = GetKB(NumSetupCopies);
+  EXPECT_EQ(0u, Res.Zero.countTrailingOnes());
+  EXPECT_EQ(64u, Res.One.countTrailingZeros());
+  EXPECT_EQ(Align(1), Info.computeKnownAlignment(Copies[5]));
+
+  CheckBits(1, NumSetupCopies + 1);
+  CheckBits(2, NumSetupCopies + 2);
+  CheckBits(3, NumSetupCopies + 3);
+  CheckBits(4, NumSetupCopies + 4);
+  CheckBits(5, NumSetupCopies + 5);
 }
 
 TEST_F(AArch64GISelMITest, TestKnownBitsUADDO) {
   StringRef MIRString = R"(
    %ptr:_(p0) = G_IMPLICIT_DEF
    %ld0:_(s32) = G_LOAD %ptr(p0) :: (load (s16))
    %ld1:_(s32) = G_LOAD %ptr(p0) :: (load (s16))