Quuxplusone
commented
20 years ago Is this just a tracking bug? There are many API's that don't work with the V9
backend...
-ChrisClosed Quuxplusone closed 14 years ago
Quuxplusone
commented
20 years ago Is this just a tracking bug? There are many API's that don't work with the V9
backend...
-ChrisIts a bug. It should work with Sparc. There is nothing machine specific about
it. If there are other APIs that don't work with Sparc they should be documented
with bug reports.Well the same could be said about just about ALL of the target interfaces the
sparc doesn't use, such as registers that are not Value*'s.
Does someone intend to implement this bug in the near future? If not, it
should probably be closed as wontfix.
-ChrisHere is my plan for this. I want to add a virtual method to TargetInstrInfo
called
"virtual bool hasDelayedBranches() const", which is basically true for sparcv9
and
false for everything else. In the case where TII.hasDelayedBranches()==false,
getFirstTerminator() will scan instructions backwards from the end of the BB, as
it does now.
In the case where TII.hasDelaySlots()==true, it will scan *every other*
instruction
from the end of the BB, assuming that if there are terminators, they will have a
single delay slot, and that the delay slots may never contain control transfer
instructions. (I think these are reasonable assumptions to make.)
Then I will set the M_TERMINATOR_FLAG on all instructions in SparcV9Instr.def
which currently have any of M_BRANCH_FLAG, M_CALL_FLAG, or M_RET_FLAG
set.
This will allow getFirstTerminator() to work on the sparc, and it will also
allow
us to zap the numDelaySlots stuff from TargetInstrDescriptor.
I welcome any constructive criticism...A couple of comments:
1. hasDelayedBranches should be part of target machine or something, not part
of target instr info. "hasDelaySlot(opcode)" would go in target instr info
2. SparcV8 has delay slots too
3. What you're really looking for is "target IMPLEMENTATION represents delay
slots in a broken way". which is a property of the target impl, not the
processor.
4. Setting M_TERMINATOR_FLAG on the appropriate instrs sounds good.
5. As I mentioned to tonic on IRC, this API doesn't really make any sense for
the V9 backend. The problem is that all clients of the IR that modify
instructions have to be aware of the delay slots already, so what does this
API actually help with? The V9 backend already doesn't have this, but it
manages to survive... what is the existing V9 code using?
Would it make sense to add a different method that did the appropriate V9 thing
to the target classes, instead of overloading this one?
-ChrisThis API makes slightly more sense than what the V9 backend currently uses,
which is the address of the first MachineInstr in the MachineCodeForInstruction
of the LLVM BasicBlock's terminator. If we were to implement this API correctly
for the V9 backend, we would be able to reduce our reliance on
MachineCodeForInstruction in the V9 Phi elimination routines, for example.
(Admittedly, this shows that getFirstTerminator could be implemented for V9
in terms of MachineCodeForInstruction, but I am not a big fan of
MachineCodeForInstruction.)I don't think that this is really a bug. It's entirely reasonable to add a
sparc specific method (with slightly different semantics) to the mbb class, but
if so it should probably be named something else.
-ChrisI beg to differ...
getFirstTerminator as it currently stands has built in to it the assumption that
all terminator instructions must be contiguous at the end of the machine basic
block. This is entirely unobvious unless you read the code for the method
itself.
If you want to keep it that way -- and that makes sense for non-delay-slot
targets! -- this needs to be documented in some way. (My apologies in advance
if I have missed something.)
In fact, one could reasonably argue that it's broken for blocks which have been
run through the DelaySlotFiller, for example (not just on ordinary SparcV9
blocks.)
In addition, the Sparc backend uses a relatively brittle and even more unobvious
method for finding the first terminator machine instr. I am still of the opinion
that migrating it to use this API (as well as other facets of the
MachineBasicBlock
class, like the machine-CFG) would be an improvement.
I'm reopening this as an enhancement bug, because although it isn't really a
show-stopper in the short term, it represents an issue that I'd like to address
in
the medium term.> getFirstTerminator as it currently stands has built in to it the assumption
that
> all terminator instructions must be contiguous at the end of the machine basic
> block. This is entirely unobvious unless you read the code for the method
> itself.
Yes, this is something that is assumed throughout the (target independent) code
generator. In particular, if there are early exits, it's not a basic block!
> If you want to keep it that way -- and that makes sense for non-delay-slot
> targets! -- this needs to be documented in some way.
Agreed, however, this has nothing to do with delay-slot vs not. The problem is
that the SparcV9 backend exposes the delay slots MUCH too early. In fact, delay
slots should only be filled at the very end of code generation, so that all
parts of the codegen don't need to know about them.
My problem with the approach of this bug is that it is confusing two things.
First, on priciple, it is always bad two name two methods that do different
things the same name. getFirstTerminator on the sparc will do something
different than it currently does on other targets (and this is not the fault of
the v9 backend!)
In fact the problem is that the getFirstTerminator method is really poorly named
for the target-independent code generators and should be fixed. The property
that current clients of this method use is that machine basic block consist of
some number of non-terminators followed by some number of terminators. The
property is not true on the V9 backend however.
The main problem, and I think the confusion surrounding this bug, is that the
existing clients really don't care about the terminatorness, they are really
just looking for the place in the mbb that has this property. If there were
some better name we could give this method, I would have no problem with the
sparc backend getting a nice shiny getFirstTerminator method: I just really
don't want them to be named the same thing.
Does this make any sense?
-ChrisThe V9 backend exposes delay slots into MBBs, which makes the concept of
'getFirstTerminator' not apply
to it (instructions can occur after terminators in blocks).
-Chris
getFirstTerminator for MachineBasicBlocks does not work for sparc